Skip to content

/ servo

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

Made http_redirect_fetch error for non-HTTPS. #14069 #14420

Merged
merged 1 commit into from Dec 1, 2016
Merged

Made http_redirect_fetch error for non-HTTPS. #14069 #14420

merged 1 commit into from Dec 1, 2016

Conversation

@46bit
Copy link
Contributor

46bit commented Nov 30, 2016 •
edited

Hi! I'm a newbie looking to resolve #14069.

The existing tests now pass.

Cheers!

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #14069 (github issue number if applicable).
  • There are tests for these changes OR

This change is Reviewable
@highfive
Copy link

highfive commented Nov 30, 2016

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

highfive commented Nov 30, 2016

Heads up! This PR modifies the following files:
@KiChjang
KiChjang requested changes Nov 30, 2016
View changes
Copy link
Member

KiChjang left a comment

Thanks for your PR! There are a couple of minor changes, r=me after they're fixed.

For the questions you posed, I don't think it's necessary to resolve them in this PR since they're out of scope. I'd also encourage you to comment on the relevant issue first before working on it next time.
components/net/http_loader.rs Outdated
// TODO implement return network_error if not HTTP(S)
match location_url.scheme() {
"http" => { },
"https" => { },

This comment has been minimized.

Sign in to view
Copy link
@KiChjang

KiChjang Nov 30, 2016

Member

This can just be "http" | "https" => { }.
components/net/http_loader.rs Outdated
match location_url.scheme() {
"http" => { },
"https" => { },
_ => return Response::network_error(NetworkError::Internal("Not an HTTPS Scheme".into()))

This comment has been minimized.

Sign in to view
Copy link
@KiChjang

KiChjang Nov 30, 2016

Member

"Not an HTTP(S) scheme".
@46bit 46bit force-pushed the 46bit:master branch from 6d64461 to 3d43b97 Nov 30, 2016
@46bit
Copy link
Contributor Author

46bit commented Nov 30, 2016 •
edited

r=@KiChjang
@KiChjang
Copy link
Member

KiChjang commented Nov 30, 2016

@46bit That message was intended for other reviewers indicating that this patch can be landed if I happen to be away for a while. Thanks for your contribution nonetheless!

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Nov 30, 2016

📌 Commit 3d43b97 has been approved by KiChjang
@bors-servo
Copy link
Contributor

bors-servo commented Nov 30, 2016

⌛ Testing commit 3d43b97 with merge 06bd113...
bors-servo added a commit that referenced this pull request Nov 30, 2016
@bors-servo
Auto merge of #14420 - 46bit:master, r=KiChjang
Loading status checks…
06bd113 
Made http_redirect_fetch error for non-HTTPS. #14069

Hi! I'm a newbie looking to resolve #14069.

The [existing tests](https://dxr.mozilla.org/servo/source/tests/wpt/web-platform-tests/fetch/api/redirect/redirect-schemes.html) now pass.

Cheers!

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14069 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
<!-- - [ ] These changes do not require tests because _____ -->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14420)
<!-- Reviewable:end -->
@bors-servo bors-servo mentioned this pull request Nov 30, 2016
Merged
4 of 4 tasks complete
@bors-servo
Copy link
Contributor

bors-servo commented Nov 30, 2016

💔 Test failed - android
@Ms2ger
Copy link
Contributor

Ms2ger commented Nov 30, 2016

@bors-servo retry servo/saltfs#545
@bors-servo
Copy link
Contributor

bors-servo commented Nov 30, 2016

⌛ Testing commit 3d43b97 with merge acdbdfa...
bors-servo added a commit that referenced this pull request Nov 30, 2016
@bors-servo
Auto merge of #14420 - 46bit:master, r=KiChjang
Loading status checks…
acdbdfa 
Made http_redirect_fetch error for non-HTTPS. #14069

Hi! I'm a newbie looking to resolve #14069.

The [existing tests](https://dxr.mozilla.org/servo/source/tests/wpt/web-platform-tests/fetch/api/redirect/redirect-schemes.html) now pass.

Cheers!

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14069 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
<!-- - [ ] These changes do not require tests because _____ -->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14420)
<!-- Reviewable:end -->
@KiChjang
KiChjang approved these changes Nov 30, 2016
View changes
@bors-servo
Copy link
Contributor

bors-servo commented Nov 30, 2016

💔 Test failed - mac-rel-wpt1
@KiChjang
Copy link
Member

KiChjang commented Nov 30, 2016

Looks like more tests are passing after your changes:

Tests with unexpected results:
  ▶ Unexpected subtest result in /fetch/api/redirect/redirect-to-dataurl.html:
  └ PASS [expected FAIL] Testing data URL loading after same-origin redirection (cors mode)

  ▶ Unexpected subtest result in /fetch/api/redirect/redirect-to-dataurl.html:
  └ PASS [expected FAIL] Testing data URL loading after same-origin redirection (no-cors mode)

  ▶ Unexpected subtest result in /fetch/api/redirect/redirect-to-dataurl.html:
  └ PASS [expected FAIL] Testing data URL loading after same-origin redirection (same-origin mode)

  ▶ Unexpected subtest result in /fetch/api/redirect/redirect-to-dataurl.html:
  └ PASS [expected FAIL] Testing data URL loading after cross-origin redirection (cors mode)

  ▶ Unexpected subtest result in /fetch/api/redirect/redirect-to-dataurl.html:
  └ PASS [expected FAIL] Testing data URL loading after cross-origin redirection (no-cors mode)

  ▶ Unexpected subtest result in /fetch/api/redirect/redirect-to-dataurl-worker.html:
  └ PASS [expected FAIL] Testing data URL loading after same-origin redirection (cors mode)

  ▶ Unexpected subtest result in /fetch/api/redirect/redirect-to-dataurl-worker.html:
  └ PASS [expected FAIL] Testing data URL loading after same-origin redirection (no-cors mode)

  ▶ Unexpected subtest result in /fetch/api/redirect/redirect-to-dataurl-worker.html:
  └ PASS [expected FAIL] Testing data URL loading after same-origin redirection (same-origin mode)

  ▶ Unexpected subtest result in /fetch/api/redirect/redirect-to-dataurl-worker.html:
  └ PASS [expected FAIL] Testing data URL loading after cross-origin redirection (cors mode)

  ▶ Unexpected subtest result in /fetch/api/redirect/redirect-to-dataurl-worker.html:
  └ PASS [expected FAIL] Testing data URL loading after cross-origin redirection (no-cors mode)

You'll need to update the test expectations accordingly.
@46bit
Made http_redirect_fetch error for non-HTTPS. #14069
Loading status checks…
52194c0
@46bit 46bit force-pushed the 46bit:master branch from 3d43b97 to 52194c0 Nov 30, 2016
@46bit
Copy link
Contributor Author

46bit commented Nov 30, 2016

Aha. I've removed the outdated expectations and checked the full ./mach test-wpt tests/wpt/web-platform-tests/fetch.
@KiChjang
Copy link
Member

KiChjang commented Nov 30, 2016

Thanks!

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Nov 30, 2016

📌 Commit 52194c0 has been approved by KiChjang
@bors-servo
Copy link
Contributor

bors-servo commented Dec 1, 2016

⌛ Testing commit 52194c0 with merge b3745d6...
bors-servo added a commit that referenced this pull request Dec 1, 2016
@bors-servo
Auto merge of #14420 - 46bit:master, r=KiChjang
Loading status checks…
b3745d6 
Made http_redirect_fetch error for non-HTTPS. #14069

Hi! I'm a newbie looking to resolve #14069.

The [existing tests](https://dxr.mozilla.org/servo/source/tests/wpt/web-platform-tests/fetch/api/redirect/redirect-schemes.html) now pass.

Cheers!

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #14069 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
<!-- - [ ] These changes do not require tests because _____ -->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14420)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 1, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit 52194c0 into servo:master Dec 1, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
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
Reviewers

@KiChjang KiChjang

Assignees

@KiChjang KiChjang

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Make HTTP redirect fetch return an error if redirecting to non-HTTP(S)
5 participants
@46bit @highfive @KiChjang @bors-servo @Ms2ger
You can’t perform that action at this time.