Skip to content

fix: basePath handling for assets#3423

Closed
fry69 wants to merge 25 commits into
freshframework:mainfrom
fry69:fry69/fix-basepath
Closed

fix: basePath handling for assets#3423
fry69 wants to merge 25 commits into
freshframework:mainfrom
fry69:fry69/fix-basepath

Conversation

@fry69

@fry69 fry69 commented Sep 13, 2025

Copy link
Copy Markdown
Contributor

trying to fix #3391 again, this gets a bit hairy, it forces propagating the basePath through the runtime.

Well, this code may not win a beauty contest, but so far it seems work and handle the mentioned cases.

Please review, check, flame me and revert my previous basePath PRs if this path is too much of a rabbit hole 😄

@fry69 fry69 marked this pull request as draft September 13, 2025 05:34
@fry69 fry69 marked this pull request as ready for review September 13, 2025 07:36
@fry69 fry69 changed the title fix: basePath handling for CSS and img links fix: basePath handling for assets and relative paths Sep 13, 2025
@fry69 fry69 changed the title fix: basePath handling for assets and relative paths fix: basePath handling for assets Sep 13, 2025
@fry69 fry69 force-pushed the fry69/fix-basepath branch from 065aea1 to b49111d Compare September 15, 2025 12:28
@bartlomieju

Copy link
Copy Markdown
Contributor

A few things I noticed:

  1. Public API change to asset() and assetSrcSet() — these are exported from shared.ts and adding a basePath parameter changes the public signature. Users calling asset("/foo.css") won't break (it's optional), but it's a leaky abstraction — why should callers need to know about basePath? Internally Fresh always has access to basePath through render state, and BUILD_ID is already set as a module-level variable. Could basePath be handled the same way instead of threading it through parameters?

  2. applyBasePath can double-prefix — there's no guard against path already starting with basePath. If someone does asset("/ui/foo.css") with basePath="/ui", they'd get /ui/ui/foo.css.

  3. Character validation regex is incomplete/@#?&=\s/ blocks some characters but allows others that are problematic in URL paths (e.g., %, {, }, <, >). Consider validating against a stricter URL path character set, or just doing new URL(basePath, "https://localhost") and checking it roundtrips cleanly.

  4. Island CSS paths — in RemainingHead(), island CSS paths (island.css[i]) get applyBasePath applied. Are these paths guaranteed to be root-relative without the base? If they already include the base from the build step, this would double-prefix them.

bartlomieju and others added 3 commits March 29, 2026 21:58
# Conflicts:
#	packages/plugin-vite/tests/build_test.ts
1. Make basePath a module-level variable (like BUILD_ID) instead of
   threading it through asset()/assetSrcSet() parameters. The public
   API no longer exposes basePath — it's set once via setBasePath()
   during app.handler() initialization.

2. Guard applyBasePath against double-prefixing — if the path already
   starts with basePath, return it unchanged.

3. Improve basePath validation — use URL round-trip check instead of
   an incomplete character regex, catching all invalid path characters.

4. Island CSS paths confirmed safe — they are root-relative from the
   build system and never include basePath, so applyBasePath (now via
   asset()) is correct.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
# Conflicts:
#	packages/fresh/src/app.ts
#	packages/plugin-vite/tests/build_test.ts

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

@bartlomieju bartlomieju enabled auto-merge (squash) March 29, 2026 20:10
The @ character is valid in URL paths, so it should no longer be
rejected. Updated test to match the new URL-based validation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@bartlomieju

Copy link
Copy Markdown
Contributor

Actually I'm gonna close this one since the original issue is now fixed.

@bartlomieju bartlomieju closed this Apr 9, 2026
auto-merge was automatically disabled April 9, 2026 07:53

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fresh basePath or Vite base property not applied correctly.

2 participants