Fix browse session authentication and UI

[SudaisAkbar]

Changes

• Fix open redirect vulnerability in browse authentication.
• Validate return_to so redirects remain on local paths.
• Replace redirect(external: ...) with safe local redirects.
• Add browse authentication controls to landing, browse, search, and item pages.
• Add logout controls and authentication state indicators across the browse UI.
• Preserve browse authentication state using the session.

[allixender]

Hi @SudaisAkbar

Thanks, this is generally the right direction. external: -> validated local path is what was originally asked for. The extraction into .auth_controls is a good decision, to avoid duplication. I checked out locally, it compiles, the tests still pass.

A few things before I can consider a merge.

1. safe_local_path/1 is a blacklist, and rather incomplete one (stac_browser_controller.ex:498)

You only reject a second leading /. Phoenix rejects three more (deps/phoenix/lib/phoenix/controller.ex:507: "\\", "/%09", "/\t"), and it does that by raising, not falling
back. Probed it:

return_to=/\evil.example       -> ** (ArgumentError) unsafe characters detected for local redirect
return_to=/<TAB>/evil.example  -> ** (ArgumentError) unsafe characters detected for local redirect

So no open redirect (Phoenix catches it), but a 500 an attacker can trigger still at will, on the exact POST that sets the session cookie. Blacklists lose this race every time. Maybe flip it to a whitelist, eg. match ~r{\A/stac/web(/|\z)} and fall back otherwise -> nothing crafted ever reaches Phoenix's raise.

2. return_to on the item page doesn't work (item.html.heex:22)

index.html.heex passes current_path={"/stac/web/browse/#{@current_path}"}, item passes bare @current_path. That's the joined route splat, collection/c1/item/i1, no leading slash -> safe_local_path drops it -> you land back on /stac/web/browse. Check/verify please.

This would mean that on item pages the control is visible but the return path is broken, I think this basically the same like described in the original issue actually. Needs the same prefix as index.

3. logout icon SVG got mangled ?

master: ...4 4H7m6 4v1a2 2 0 01-2 2H6a2 2 0 01-2-2V7a2 2 0 012-2h5...
PR:     ...4 4H7m6 4v1a2 2 0 01-2-2V7a2 2 0 012-2h5...

2H6a2 2 0 dropped, that's the door outline. Renders distorted now. Copy/paste slip I assume? Please repair.

4. No tests. The four existing browse-auth tests are untouched and green, but the thing this PR exists to fix is not covered. Nothing stops the next refactor from quietly putting external: back. Three cases is enough: absolute URL, //host, /\host. Please add them, security fixes don't get merged here on manual verification alone.

5. Please keep mix format out of a feature/bug fixing PR. most of the changed lines are reformatting of files you happened to touch, and the ~15 lines that actually matter are buried in it. I had to figure out the real change by hand to review it. This is not ok. Leave it for now, but don't do random formatting in future PRs.

Small polishing you should include:

• landing/2 and both branches of search/2 re-assign :browse_authenticated, the plug :assign_browse_authenticated already sets it on every action -> dead code
• auth_controls/1 has no attr declarations, so no compile-time check on the two required assigns
• logout now returns to the current page, so logging out on a private catalog page gives a "Not found" flash rather than "logged out". Confusing? Maybe just always send logout to /stac/web/browse

Ping me when it's pushed, happy to look again quickly.

[SudaisAkbar]

Hi, I’ve addressed the review comments:

Fixed the return_to validation and added security tests.
Fixed the item-page return path.
Repaired the logout icon.
Removed redundant authentication assignments.
Added required attributes to auth_controls.
Logout now redirects to the browse page.
Added a doc for more clarification.

Thank you for checking again!