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

Fix error-index redirect to work with the back button. #106491

Merged
merged 2 commits into from
Jan 6, 2023

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jan 5, 2023

This fixes the redirect page at https://doc.rust-lang.org/error-index.html so that it works with the browser's back-button. The solution is to use window.location.replace(), which avoids adding the page to the browser history stack.

This also cleans up the code a little bit, since it looks like it was written with different assumptions (maybe before f5857d5). I don't think there is a need to have a redirect at https://doc.rust-lang.org/error_codes/error-index.html since I don't think fragment-based links were ever used there.

I have tested with Firefox, Chrome, and Safari. Going to /error-index.html redirects without adding to the stack, and can use the back button. Additionally, /error-index.html#E0005 redirects correctly to the error page.

Finally, there is an unrelated commit to remove the 404 hack. There is an official way of avoiding the generation of the 404 page with setting input-404 to an empty value.

Fixes #106485

@rustbot
Copy link
Collaborator

rustbot commented Jan 5, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 5, 2023
@ehuss
Copy link
Contributor Author

ehuss commented Jan 5, 2023

cc @GuillaumeGomez

Comment on lines 4 to 5
// We have to make sure this pattern matches to avoid inadvertently creating an
// open redirect.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please preserve this comment. It's important that the official rust-lang.org website, which people visit to download the compiler, not host any open redirects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, good point, thanks!

// We have to make sure this pattern matches to avoid
// inadvertently creating an open redirect.
if (/^E[0-9]+$/.test(code)) {
window.location.replace('./error_codes/' + code + '.html');
Copy link
Member

Choose a reason for hiding this comment

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

Why not making this change in the JS file directly? It had the advantage of keeping things split (JS split from HTML) which is easier to read/maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, no particular reason. It was no longer being shared between different pages, and it seemed a little more convenient to have everything in one place. I don't have a strong preference, though, so I pushed an update to keep it in the old location.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! It also makes the changes much smaller. :)

@@ -7,6 +7,7 @@ src = ""
git-repository-url = "https://github.com/rust-lang/rust/"
additional-css = ["error-index.css"]
additional-js = ["error-index.js"]
input-404 = ""
Copy link
Member

Choose a reason for hiding this comment

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

Nice improvement!

@GuillaumeGomez
Copy link
Member

Looks good to me. Thanks for the improvement!

@notriddle
Copy link
Contributor

r? notriddle

@bors r=GuillaumeGomez,notriddle rollup

@bors
Copy link
Contributor

bors commented Jan 5, 2023

📌 Commit 72c3082 has been approved by GuillaumeGomez,notriddle

It is now in the queue for this repository.

@rustbot rustbot assigned notriddle and unassigned Mark-Simulacrum Jan 5, 2023
@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 5, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 6, 2023
Rollup of 5 pull requests

Successful merges:

 - rust-lang#106400 (Point at expressions where inference refines an unexpected type)
 - rust-lang#106491 (Fix error-index redirect to work with the back button.)
 - rust-lang#106494 (Add regression test for rust-lang#58355)
 - rust-lang#106499 (fix [type error] for error E0029 and E0277)
 - rust-lang#106502 (rustdoc: remove legacy user-select CSS)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2b6e38c into rust-lang:master Jan 6, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Back button doesn't work from error_codes/error-index.html
6 participants