Repository navigation
fix(fonts): deny a Google Fonts fetch unless it is opted into - #420
Merged
Merged
Conversation
A scenario declaring fonts: [{ source: "google", ... }] made validate
and still reach fonts.googleapis.com and write the result into the font
cache. Neither command asked. The declared consumer of a scenario is a
model generating JSON, so the file is untrusted input, and it is the
file that chooses the target of the request: validating someone else's
scenario should not be a network operation, and on a CI runner it turns
a lint step into an outbound fetch.
RemoteFontPolicy mirrors RemoteIncludePolicy rather than inventing a
second authorization vocabulary: Deny by default, one flag to opt in.
--allow-remote-fonts is global, so it works on any subcommand.
The refusal sits immediately before the first network call, not at the
top of the resolver. So it fires exactly when a request would leave, and
a family already in the cache needs no flag at all -- an offline render
that has its fonts keeps working, which is what makes this safe to
default to deny. Only the weights actually missing from the cache are
named.
The policy is a process-level atomic rather than a parameter because
resolve_font_entry is reached through load_custom_fonts from eight call
sites, none of which carry a policy today.
Not in scope, and worth its own issue: --allow-remote-include is named
in the include refusal but does not exist as a CLI flag. The loader
hardcodes RemoteIncludePolicy::Deny, so the message points at a flag
nobody can pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #320.
The defect
A scenario declaring
fonts: [{ source: "google", ... }]madevalidateandstillreachfonts.googleapis.comand write the result into the font cache.Neither command asked.
The declared consumer of a scenario is a model generating JSON, so the file is
untrusted input — and it is the file that chooses the target of the request.
Validating someone else's scenario should not be a network operation, and on a CI
runner it turns a lint step into an outbound fetch.
Shape
RemoteFontPolicymirrorsRemoteIncludePolicyrather than inventing a secondauthorization vocabulary:
Denyby default, one flag to opt in.--allow-remote-fontsis global, so it works on any subcommand.Where the check sits, and why it matters
Immediately before the first network call, not at the top of the resolver. So it
fires exactly when a request would leave, and a family already in the cache
needs no flag at all — an offline render that has its fonts keeps working, which
is what makes deny-by-default safe to ship. Only the weights actually missing from
the cache are named.
Verified end to end: refused without the flag (no request), fetched and cached
with it, then accepted again without it once cached.
Why a process-level policy
resolve_font_entryis reached throughload_custom_fontsfrom eight call sites,none of which carry a policy today. Threading a parameter through all of them buys
nothing over an atomic set once from
run().Not in scope, worth its own issue
--allow-remote-includeis named in the include refusal but does not exist as aCLI flag. The loader hardcodes
RemoteIncludePolicy::Deny, so that messagepoints at a flag nobody can pass. Wiring it means threading the policy through
load_scenario_from_source, which is a different change from this one.Tests
Four, including
a_family_already_in_the_cache_needs_no_flagandonly_the_weights_that_are_missing_are_named_in_the_refusal. Removing the checkfails two of them — and, during the revert, actually performs the fetch, which is
the behaviour being closed.
Gate
cargo fmt --all --checkclean ·cargo clippy --workspace --all-targets --features rustmotion/studio -D warningsclean ·cargo test --workspace1807 passed.