-
Notifications
You must be signed in to change notification settings - Fork 365
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Some of this appears to have been already covered in PR #663 #670
Conversation
@kwwall I'm researching the build failure. I didn't touch anything in encryption, so it seems odd. |
Can you email me the 'mvn -B test' output and the failed .txt files under
target/surefire-reports?
And maybe the output of 'mvn B dependency:tree' as well.
Thanks,
-kevin
…On Sat, Mar 19, 2022 at 2:42 PM Matt Seil ***@***.***> wrote:
@kwwall <https://github.com/kwwall> I'm researching the build failure. I
didn't touch anything in encryption, so it seems odd.
—
Reply to this email directly, view it on GitHub
<#670 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAO6PG2AO2I6V67KEIDRRBTVAYN2XANCNFSM5REK5IFA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Blog: https://off-the-wall-security.blogspot.com/ | Twitter: @KevinWWall
| OWASP ESAPI Project co-lead
NSA: All your crypto bit are belong to us.
|
@xeno6696 - Never mind. I thought you meant personal tests. I wonder what the |
It’s something I missed: I got so used to ignoring encryptor tests I only tested the classes I touched. This is related to the Cookie. Half-jokingly, it’s on the Authenticator api so my gut wants to just delete it. (I don’t think anyone, anywhere uses it.) but I’ll be good and straighten it out. |
@xeno6696 - That's good that you're looking at it bc I really need to start looking at my taxes. I haven't even started started on them and I need to get them to my CPA no later than Friday or so. |
Should be good to go whenever you're satisfied @kwwall If my branch still isn't syncing properly, I'll just kill my fork and reclone. |
… new regression tests for other purposes in validation and encoder tests.
@kwwall @jeremiahjstacey just leaving a breadcrumb here. I'd like someone else to validate that dropping @ignore as noted in the last commit still results in a DOS. |
… opened up on github. One of which however reminded me that we need a codec to account for UTF-8 encoding/decoding.
Validator v = ESAPI.validator(); | ||
boolean expected = false; | ||
boolean result = v.isValidInput("HTTPHeaderValue ", "[email protected]", "HTTPHeaderValue", 2147483647, true, true); | ||
assertEquals(expected, result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, why not just assertFalse()
here instead of setting expected
to false and using assertEquals()
here? I'd recommend parting it after the previous test (lines 1128-1129).
Other than running the tests until it crashes with an OutOfMemoryError, or running it with a debugger, how would you propose to do that? DoS vulnerabilities are generally hard to write unit tests for anyway. |
String expected = new String (new int[]{0x2f804}, 0, 1); | ||
assertEquals( expected, htmlCodec.decode("你") ); | ||
assertEquals( expected, htmlCodec.decode("你") ); | ||
} | ||
|
||
public void testUnicodeCanonicalize() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should at least add a comment here to refer to the appropriate GitHub issue #.
What branch are you using for your upstream? 'develop' or 'main'?
Also why is commit f684e2a say:
This commit does not belong to any branch on this repository, and may
belong to a fork outside of the repository.
Browse files
<https://github.com/ESAPI/esapi-java-legacy/tree/f684e2aaf5cb1e8a0ea36c49e893101285e8fb5a>
When PR #663 was merged, did you accidentally leave that commit off the PR?
Because I don't see it listed on
https://github.com/ESAPI/esapi-java-legacy/pull/663/commits
…-kevin
On Sat, Mar 19, 2022 at 5:06 PM Matt Seil ***@***.***> wrote:
Should be good to go whenever you're satisfied @kwwall
<https://github.com/kwwall>
If my branch still isn't syncing properly, I'll just kill my fork and
reclone.
—
Reply to this email directly, view it on GitHub
<#670 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAO6PG4NKUYI3THVGCLZTHDVAY6V3ANCNFSM5REK5IFA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Blog: https://off-the-wall-security.blogspot.com/ | Twitter: @KevinWWall
| OWASP ESAPI Project co-lead
NSA: All your crypto bit are belong to us.
|
My github repo still shows that I'm ahead of ESAPI by 21 commits, which includes all of these. This makes no sense since @kwwall shows it was merged.
At any rate, only the two commits from today are any different from PR #663