Skip to content
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

Support 'report-sample' keyword-source #184

Merged
merged 1 commit into from
Aug 11, 2017
Merged

Support 'report-sample' keyword-source #184

merged 1 commit into from
Aug 11, 2017

Conversation

shekyan
Copy link
Collaborator

@shekyan shekyan commented Jul 31, 2017

fixes #183

@shekyan shekyan requested a review from michaelficarra July 31, 2017 04:54
@shekyan shekyan force-pushed the report-sample branch 3 times, most recently from c84efaa to 0e2706a Compare July 31, 2017 06:44
p = parseWithNotices("style-src 'report-sample' 'unsafe-hashed-attributes' 'report-sample' 'nonce-123'", notices);
assertEquals(1, p.getDirectives().size());
assertEquals(2, notices.size());
assertEquals("Invalid base64-value (should be multiple of 4 bytes: 3).", notices.get(0).message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have two notices and the one you're asserting on is unrelated to report-sample. Just fix the nonce and assert on the remaining notice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both notices are unrelated to report-sample, removed the test

p = parseWithNotices("script-src 'report-sample' 'report-sample'", notices);
assertEquals(1, p.getDirectives().size());
assertEquals("script-src 'report-sample'", p.getDirectiveByType(ScriptSrcDirective.class).show());
assertEquals(0, notices.size());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two report-samples, but no notices?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be addressed in #185

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to seem some expected parse failures in the tests. Try using report-sample outside of a source expression list.

@@ -391,6 +391,8 @@ private void enforceMissingDirectiveValue(@Nonnull Token directiveNameToken) thr
seenStates.add(SeenStates.SEEN_HOST_OR_SCHEME_SOURCE);
} else if (se == KeywordSource.UnsafeHashedAttributes) {
seenStates.add(SeenStates.SEEN_UNSAFE_HASHED_ATTR);
} else if (se == KeywordSource.ReportSample) {
seenStates.add(SeenStates.SEEN_REPORT_SAMPLE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems silly to add this if it's not going to be read, but whatever.

@shekyan shekyan merged commit a88034e into master Aug 11, 2017
@shekyan shekyan deleted the report-sample branch August 11, 2017 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support report-sample keyword-source
2 participants