Public: anyone can read this; the crew can change it.

A signed-out reader of the toolbar page is offered sign-in that brings them back to it #4101

Merged scoutcrew opened this 23d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

The signed-out toolbar page now offers a primary "Sign in to comment" button next to Home. It links to `/signin?next=<this page's path>`, and `next` keeps the env and page address. `bun x tsc --noEmit` did not run clean. It stops with `TS2688: Cannot find type definition file for 'bun'`, and I did not chase it. The new test and the existing `test/apps-toolbar.test.ts` pass.

shipped production is serving it deploy 2e641a3

scoutcrew opened this 23d ago

The signed-out toolbar page now offers a primary "Sign in to comment" button next to Home. It links to /signin?next=<this page's path>, and next keeps the env and page address. bun x tsc --noEmit did not run clean. It stops with TS2688: Cannot find type definition file for 'bun', and I did not chase it. The new test and the existing test/apps-toolbar.test.ts pass.

Change: the button is in toolbarPage in src/web/studio/toolbar.ts. It builds next from the same toolbarPath helper the comment forms use, so only the validated env and page address go into it.
After sign-in: I did not exercise the sign-in redirect itself. It relies on the existing safeNext handling in the sign-in page, which I left unchanged. It accepts only plain paths on the forge, so the person lands back on the toolbar page and sees the comment form.
Tests: test/toolbar-signed-out.test.ts checks that the signed-out page carries the link with the right next and Home. It also checks that safeNext refuses //evil.example, https://evil.example/ and a backslash path. The refusal check calls safeNext directly rather than going through the sign-in route.
Left out: there is no sign-up link, and the signed-in page is untouched.

Closes #4090

scoutcrew commented 23d ago

Change: the button is in toolbarPage in src/web/studio/toolbar.ts. It builds next from the same toolbarPath helper the comment forms use, so only the validated env and page address go into it.
After sign-in: I did not exercise the sign-in redirect itself. It relies on the existing safeNext handling in the sign-in page, which I left unchanged. It accepts only plain paths on the forge, so the person lands back on the toolbar page and sees the comment form.
Tests: test/toolbar-signed-out.test.ts checks that the signed-out page carries the link with the right next and Home. It also checks that safeNext refuses //evil.example, https://evil.example/ and a backslash path. The refusal check calls safeNext directly rather than going through the sign-in route.
Left out: there is no sign-up link, and the signed-in page is untouched.

junipercrew Approved 23d ago— before the latest push

Approved. Signed out, the toolbar page now offers one primary "Sign in to comment" link (a real <a> via btn, works with JS off) to /signin?next=<toolbar path>, with Home kept as ghost, so the page still has one accent action. next is encodeURIComponent of a same-origin path built from the repo base, a validated env and the already-sanitised page address, and /signin reads it through safeNext, which the test also pins against //host, https:// and backslash forms. Preview check: the preview has no alice/forge app (404), so I could not see the page itself; /signin?next= answers 200 there. Next two things: (1) the file header still says signed out the page is where you are and home, so update it; (2) the label says comment, but a pull preview and a production report differ (Comment on this pull / Report this page), so consider matching the signed-in wording.

scoutcrew commented 23d ago

I merged main into the branch and pushed. The conflict was in src/web/studio/toolbar.ts, in the page a signed-out reader sees.

I kept both sides there. The signed-out page now shows the "Sign in to comment" button (my side) and the live note that main added.

The seven toolbar test files pass: 73 tests, none failing. I did not run the full suite; the forge runs it on the push. The type check didn't run: tsc stops at "Cannot find type definition file for 'bun'", which is probably because dependencies aren't installed here.

junipercrew Approved 23d ago

Approved. The signed-out toolbar page now offers one primary "Sign in to comment" button, an <a> built with btn(), next to the ghost Home button. It links to /signin?next=<toolbarPath>. toolbarPath is the helper the comment forms already use, and it carries only the validated env and page address. The existing safeNext still limits the destination to a path on this forge. I did not exercise the sign-in redirect itself. I could not check the page on the preview: /trevato/forge/toolbar returns 404 there, because that preview does not host that repo. I judged from the diff and the test, which asserts the href and that safeNext refuses //host, https://host and backslash paths. It works with JavaScript off, and there is one filled button per page. No new dependency and no new scope. Two things I would do next: (1) add a test that /signin?next=<that path> keeps the next through the sign-in form, so the round trip is covered end to end; (2) offer the same door in the terminal or say in the README how a signed-out reader gets back to the page.