Fix issue with 404 errors on preview, make the client side fetch actually use configured base path

This commit is contained in:
mrkmntal 2026-09-29 14:07:23 -04:00
commit e6519d42a4
5 changed files with 90 additions and 4 deletions

View file

@ -383,7 +383,18 @@ Consequences that are easy to get wrong:
- **Cookie `Path` is scoped to the prefix** (`Path=/forgejo`), so the session token is not sent to sibling apps on the same host. At the root it is `/`, the default-path. - **Cookie `Path` is scoped to the prefix** (`Path=/forgejo`), so the session token is not sent to sibling apps on the same host. At the root it is `/`, the default-path.
- **Static assets stay app-absolute** (`nest_service("/static")` in `routes::app`). This is the one place the base path is *not* applied, and getting it wrong is subtle: the asset 404s, `x-data="app()"` never initialises, and every Alpine directive — including the File menu dropdowns — silently stops working. There is a test asserting `GET /static/app.js` returns 200 while a base path is set. `app.js` is inlined via `include_str!` and has no URL to rewrite; only `alpine.min.js` and `style.css` are externally loaded. - **Static assets stay app-absolute** (`nest_service("/static")` in `routes::app`). This is the one place the base path is *not* applied, and getting it wrong is subtle: the asset 404s, `x-data="app()"` never initialises, and every Alpine directive — including the File menu dropdowns — silently stops working. There is a test asserting `GET /static/app.js` returns 200 while a base path is set. `app.js` is inlined via `include_str!` and has no URL to rewrite; only `alpine.min.js` and `style.css` are externally loaded.
- **Curl will 404 on prefixed paths.** Hitting `http://host:3000/forgejo/login` directly bypasses Apache, so nothing strips the prefix. Test through the proxy, or curl the app-absolute path. - **Curl will 404 on prefixed paths.** Hitting `http://host:3000/forgejo/login` directly bypasses Apache, so nothing strips the prefix. Test through the proxy, or curl the app-absolute path.
- **Never hardcode a path in JS.** `static/app.js` matches the new-note form with `form[action$="/notes"]` rather than an exact attribute, so `Ctrl+N` keeps working under any prefix. - **Never hardcode a path in JS.** `static/app.js` matches the new-note form with `form[action$="/notes"]` rather than an exact attribute, so `Ctrl+N` keeps working under any prefix. The one `fetch()` it makes is different, because the rule above does not cover it:
```javascript
const base = window.__BASE_PATH__ || '';
fetch(base + '/preview', { ... });
```
`app.js` is inlined into the page, so it cannot call the server-side `base.url()` helper that every form action and asset `src` uses. A literal `fetch('/preview')` resolves against the *origin root*, which behind a proxy mounted at `/forgejo/` never reaches the app. The failure is nasty because the proxy returns its own 404 body, which `x-html` then swaps straight into the preview pane — the user sees "404 Not Found" in the middle of their document while typing, with nothing in the server logs to explain it. `render_app` therefore injects `window.__BASE_PATH__` next to `__INITIAL_PREVIEW__`, escaped by the same `escape_js` helper and emitted as a backtick literal like its neighbour.
Pinned by `the_base_path_is_handed_to_client_side_js` and `the_preview_fetch_uses_the_injected_base_path` in `tests/auth_gating.rs`. The second matters most: `app.js` is inlined verbatim, so a regression there is invisible to every other test in the suite.
- **A redirect target is not a forwarded path.** A `303` to `{base}/` is correct for the browser and 404s if you curl it directly, because nothing strips the prefix. This also bites reverse-proxy test harnesses: a proxy built on `urllib`/`requests` will silently follow the redirect, re-resolve it against the upstream root, and return a 404 that looks like an app bug. Disable redirect following when testing through a proxy.
--- ---

View file

@ -70,8 +70,8 @@ cargo run -- --base-path /forgejo
``` ```
```apache ```apache
ProxyPass /forgejo/ http://127.0.0.1:3000/ timeout=10 ProxyPass /el-notes/ http://127.0.0.1:3000/ timeout=10
ProxyPassReverse /forgejo/ http://127.0.0.1:3000/ ProxyPassReverse /el-notes/ http://127.0.0.1:3000/
``` ```
The proxy strips the prefix, so the app still sees ordinary paths and adds the prefix back when generating links, form actions, and redirects. The session cookie's `Path` is scoped to the prefix, so the token is not sent to sibling apps on the same host. The proxy strips the prefix, so the app still sees ordinary paths and adds the prefix back when generating links, form actions, and redirects. The session cookie's `Path` is scoped to the prefix, so the token is not sent to sibling apps on the same host.

View file

@ -21,6 +21,17 @@ impl<'a> Base<'a> {
pub fn url(&self, path: &str) -> String { pub fn url(&self, path: &str) -> String {
format!("{}{}", self.0, path) format!("{}{}", self.0, path)
} }
/// The raw prefix, for handing to client-side JS.
///
/// `url()` covers every URL in the rendered markup. This exists for the
/// one `fetch()` in `app.js`, which has to build the URL client-side and
/// would otherwise resolve `/preview` against the origin root — behind a
/// reverse proxy mounted at a subdirectory that misses the app entirely
/// and returns the proxy's own 404.
pub fn prefix(&self) -> &str {
self.0
}
} }
pub fn render_app( pub fn render_app(
@ -72,6 +83,7 @@ pub fn render_app(
(render_status_bar(notes.len(), theme_name)) (render_status_bar(notes.len(), theme_name))
script { script {
"window.__INITIAL_PREVIEW__ = `" (PreEscaped(escape_js(&rendered_preview))) "`;" "window.__INITIAL_PREVIEW__ = `" (PreEscaped(escape_js(&rendered_preview))) "`;"
"window.__BASE_PATH__ = `" (PreEscaped(escape_js(base.prefix()))) "`;"
(PreEscaped(include_str!("../static/app.js"))) (PreEscaped(include_str!("../static/app.js")))
} }
} }

View file

@ -53,7 +53,16 @@ function app() {
const formData = new URLSearchParams(); const formData = new URLSearchParams();
formData.append('content', content); formData.append('content', content);
fetch('/preview', { // The base path has to be prepended here. `app.js` is inlined into
// the page, so it cannot call the server-side `base.url()` helper
// that every form action uses — and an origin-absolute `/preview`
// resolves against the site root, which behind a reverse proxy
// mounted at a subdirectory never reaches the app. The response body
// would then be the proxy's own 404, swapped straight into the
// preview pane.
const base = window.__BASE_PATH__ || '';
fetch(base + '/preview', {
method: 'POST', method: 'POST',
headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, headers: { 'Content-Type': 'application/x-www-form-urlencoded' },
body: formData.toString() body: formData.toString()

View file

@ -720,6 +720,60 @@ async fn every_screen_declares_a_viewport() {
} }
} }
/// `app.js` is inlined into the page, so it cannot call the server-side
/// `base.url()` helper. Its one `fetch()` has to build the URL itself, which
/// is what `window.__BASE_PATH__` is for.
///
/// Without it, `fetch('/preview')` resolves against the origin root. Behind
/// a reverse proxy mounted at a subdirectory that never reaches the app, and
/// the proxy's own 404 body gets swapped into the preview pane. The symptom
/// is a preview pane full of "404 Not Found" while typing.
#[tokio::test]
async fn the_base_path_is_handed_to_client_side_js() {
// With a base path set, the browser must be told about it.
let f = Fixture::with_base_path("base-js", "/forgejo")
.login_over_http()
.await;
let (status, body) = f.get_authed("/").await;
assert_eq!(status, StatusCode::OK);
// Backtick template literal, matching the `__INITIAL_PREVIEW__` line
// above it and escaped by the same `escape_js` helper.
assert!(
body.contains("window.__BASE_PATH__ = `/forgejo`;"),
"client JS should be handed the base path"
);
// Without one, the value must be empty rather than the string "undefined",
// so the fetch URL stays byte-identical to the pre-fix behaviour.
let plain = Fixture::fresh("base-js-none").login_over_http().await;
let (status, body) = plain.get_authed("/").await;
assert_eq!(status, StatusCode::OK);
assert!(
body.contains("window.__BASE_PATH__ = ``;"),
"with no base path the client value should be empty"
);
}
/// The `fetch()` in `app.js` must actually use the injected value. This is
/// the assertion that would have caught the original bug: `app.js` is
/// inlined verbatim, so a regression there is invisible to every other test.
#[tokio::test]
async fn the_preview_fetch_uses_the_injected_base_path() {
let f = Fixture::with_base_path("fetch-base", "/forgejo")
.login_over_http()
.await;
let (status, body) = f.get_authed("/").await;
assert_eq!(status, StatusCode::OK);
assert!(
body.contains("fetch(base + '/preview'"),
"app.js should build the preview URL from __BASE_PATH__"
);
assert!(
!body.contains("fetch('/preview'"),
"app.js must not hardcode an origin-absolute /preview"
);
}
/// The about page advertises a version, and it must be the one in /// The about page advertises a version, and it must be the one in
/// `Cargo.toml`. /// `Cargo.toml`.
/// ///