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

fixes 4184: no-sniff and check-for-apache-bug for mime sniffing #6171

Merged
merged 2 commits into from May 26, 2015

Conversation

@eleweek
Copy link
Contributor

eleweek commented May 24, 2015

I tried fixing #4184 , here is the code I have right now.

I haven't tested it, because I don't know what is the best way to test this part of code. Would like some help with testing this. Should I write an autotest or should I just test manually?

Review on Reviewable

@highfive
Copy link

highfive commented May 24, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Ms2ger (or someone else) soon.

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented May 24, 2015

Critic review: https://critic.hoppipolla.co.uk/r/5090

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.


if let Some(ref headers) = metadata.headers {
if let Some(ref raw_content_type) = headers.get_raw("content-type") {
let ref last_raw_content_type = raw_content_type[raw_content_type.len() - 1];

This comment has been minimized.

Copy link
@Manishearth

Manishearth May 24, 2015

Member

what if len() is zero?

@Manishearth
Copy link
Member

Manishearth commented May 24, 2015

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.
Reviewed files:

  • components/net/resource_task.rs @ r1

components/net/resource_task.rs, line 113 [r1] (raw file):
Shouldn't this be X-content-type-options?


components/net/resource_task.rs, line 115 [r1] (raw file):
Nit: nosniff = raw_blah.iter().any(|ref opt| opt == b"nosniff") is more idiomatic.


Comments from the review on Reviewable.io

@Manishearth
Copy link
Member

Manishearth commented May 24, 2015

Filed web-platform-tests/wpt#1851 for the tests

@jdm
Copy link
Member

jdm commented May 25, 2015

In terms of automated tests, there's the fetch/nosniff directory in WPT. These can be enabled by adding the dir to http://mxr.mozilla.org/servo/source/tests/wpt/include.ini and run via ./mach test-wpt /fetch/nosniff/, but we don't enable MIME sniffing by default right now so they'll still fail. You could switch that in http://mxr.mozilla.org/servo/source/components/util/opts.rs#405 of course, but I don't think we actually want to turn it on by default right now.

@eleweek
Copy link
Contributor Author

eleweek commented May 25, 2015

I updated the PR, I tested the code manually(both flags are set correctly). According to the discussion above there are things that needs to be done before enabling automatic testing, so I didn't do anything about this.

@Manishearth
Copy link
Member

Manishearth commented May 26, 2015

Review status: :shipit: all files reviewed, all discussions resolved, all commit checks successful.
Reviewed files:

  • components/net/resource_task.rs @ r2

Comments from the review on Reviewable.io

@Manishearth
Copy link
Member

Manishearth commented May 26, 2015

@bors-servo: r+

thanks!

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2015

📌 Commit f5a0285 has been approved by Manishearth

@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2015

Testing commit f5a0285 with merge be6c251...

bors-servo pushed a commit that referenced this pull request May 26, 2015
I tried fixing #4184 , here is the code I have right now. 

I haven't tested it, because I don't know what is the best way to test this part of code. Would like some help with testing this. Should I write an autotest or should I just test manually?

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6171)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 26, 2015

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

@bors-servo bors-servo merged commit f5a0285 into servo:master May 26, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.