Skip to content

rustdoc template font links only emit crossorigin when needed #144467

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MingweiSamuel
Copy link
Contributor

@MingweiSamuel MingweiSamuel commented Jul 25, 2025

The crossorigin attribute may cause issues when the href is not actually cross-origin. Specifically, the tag causes the browser to send a preflight OPTIONS request to the server even if it is same-origin. Some temperamental servers may reject all CORS preflight requests even if they're actually same-origin, which causes a CORS error and prevents the fonts from loading, even later on.

This commit fixes that problem by not emitting crossorigin if the url appears to be relative to the same origin.

@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2025

r? @GuillaumeGomez

rustbot has assigned @GuillaumeGomez.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Jul 25, 2025
@GuillaumeGomez
Copy link
Member

Seems ok to. Do you see any potential issue @notriddle?

@MingweiSamuel MingweiSamuel marked this pull request as draft July 27, 2025 17:57
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2025
@MingweiSamuel MingweiSamuel force-pushed the users/mingwes/rustdoc-fix-cors branch 2 times, most recently from 7db45e4 to 24eb433 Compare July 28, 2025 16:40
@MingweiSamuel MingweiSamuel changed the title rustdoc layout template only emit crossorigin when needed rustdoc template font links only emit crossorigin when needed Jul 28, 2025
@MingweiSamuel MingweiSamuel marked this pull request as ready for review July 28, 2025 16:41
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 28, 2025
@MingweiSamuel
Copy link
Contributor Author

Updated check should be a lot more robust, and adjusted naming/comments to discourage other [mis]usage

@MingweiSamuel

This comment was marked as outdated.

@MingweiSamuel MingweiSamuel force-pushed the users/mingwes/rustdoc-fix-cors branch from f462760 to f10e91e Compare July 29, 2025 16:38
@MingweiSamuel
Copy link
Contributor Author

@MingweiSamuel
Copy link
Contributor Author

@GuillaumeGomez Can you take another look? Should properly handle all schemes based on the spec now

Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

You removed the tests. ^^'

@MingweiSamuel MingweiSamuel force-pushed the users/mingwes/rustdoc-fix-cors branch from 89df295 to f516d4c Compare August 1, 2025 21:42
@MingweiSamuel
Copy link
Contributor Author

Alright I have added the unit tests (though I am unsure how to run them locally)

@GuillaumeGomez
Copy link
Member

You can run unit tests in rustdoc like this: x.py test src/librustdoc --stage 1 (--stage 1 is to prevent building more than necessary).

@GuillaumeGomez
Copy link
Member

Just one last nit and we're there. :)

The `crossorigin` attribute may cause issues when the href is not actuall across origins. Specifically, the tag causes the browser to send a preflight OPTIONS request to the href even if it is same-origin.
Some tempermental servers may reject all CORS preflect requests even if they're actually same-origin, which causes a CORS error and prevents the fonts from loading, even later on.

This commit fixes that problem by not emitting `crossorigin` if the url looks like a domain-relative url.

Co-authored-by: Guillaume Gomez <[email protected]>
@MingweiSamuel MingweiSamuel force-pushed the users/mingwes/rustdoc-fix-cors branch from f516d4c to 340984b Compare August 2, 2025 17:26
@rust-log-analyzer
Copy link
Collaborator

The job tidy failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
fmt check
fmt: checked 6289 files
tidy check
Checking tidy rustdoc_json...
tidy error: `/checkout/src/librustdoc/html/layout.rs:166` contains `#[test]`; unit tests and benchmarks must be placed into separate files or directories named `tests.rs`, `benches.rs`, `tests` or `benches`
No error code explanation was removed!
tidy: Skipping binary file check, read-only filesystem
removing old virtual environment
creating virtual environment at '/checkout/obj/build/venv' using 'python3.10' and 'venv'
creating virtual environment at '/checkout/obj/build/venv' using 'python3.10' and 'virtualenv'
---

added 291 packages in 37s
Running eslint on rustdoc JS files
info: ES-Check: there were no ES version matching errors!  🎉
some tidy checks failed
Command has failed. Rerun with -v to see more details.
Build completed unsuccessfully in 0:01:56
  local time: Sat Aug  2 17:32:37 UTC 2025
  network time: Sat, 02 Aug 2025 17:32:37 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@MingweiSamuel
Copy link
Contributor Author

Hmm 😅

@GuillaumeGomez
Copy link
Member

Arf, sorry about that. I definitely don't like this approach but if it's the one enforced in the project...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants