Skip to content

Mark account routes as internal and include in schema#12264

Merged
RayBB merged 2 commits intointernetarchive:masterfrom
Sadashii:fix/mark-account-routes-as-internal
Apr 3, 2026
Merged

Mark account routes as internal and include in schema#12264
RayBB merged 2 commits intointernetarchive:masterfrom
Sadashii:fix/mark-account-routes-as-internal

Conversation

@Sadashii
Copy link
Copy Markdown
Contributor

@Sadashii Sadashii commented Apr 2, 2026

Closes #

Mark 3 routes in fastapi accounts as internal

Technical

Testing

Screenshot

Stakeholders

@RayBB

Copilot AI review requested due to automatic review settings April 2, 2026 06:10
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Marks three FastAPI account debugging/auth test routes as “internal” and conditionally includes them in the OpenAPI schema based on a local-dev environment flag.

Changes:

  • Adds a SHOW_INTERNAL_IN_SCHEMA flag derived from LOCAL_DEV.
  • Tags /account/test.json, /account/protected.json, and /account/optional.json as internal.
  • Hides those endpoints from the OpenAPI schema unless running in local dev.


router = APIRouter()

SHOW_INTERNAL_IN_SCHEMA = os.getenv("LOCAL_DEV") is not None
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

SHOW_INTERNAL_IN_SCHEMA = os.getenv("LOCAL_DEV") is not None treats LOCAL_DEV=false (or any non-empty value) as enabled, which diverges from the rest of the FastAPI app where LOCAL_DEV is parsed as a boolean string (e.g., openlibrary/asgi_app.py:164 checks == "true"). This can unintentionally expose internal endpoints in the OpenAPI schema in environments that set LOCAL_DEV to a falsey string. Consider normalizing this to the same boolean parsing logic used elsewhere (e.g., .get(...).lower() == "true").

Suggested change
SHOW_INTERNAL_IN_SCHEMA = os.getenv("LOCAL_DEV") is not None
SHOW_INTERNAL_IN_SCHEMA = os.environ.get("LOCAL_DEV", "false").lower() == "true"

Copilot uses AI. Check for mistakes.

# TODO: Delete this before merging, it's just for local testing for now.
@router.get("/account/test.json", response_model=AuthTestResponse)
@router.get("/account/test.json", response_model=AuthTestResponse, tags=["internal"], include_in_schema=SHOW_INTERNAL_IN_SCHEMA)
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

/account/test.json returns the raw session cookie value (and parsed components) back to the client. Since the session cookie is set HttpOnly, echoing it via an API endpoint defeats that protection and makes session exfiltration easier if any same-origin XSS occurs. If this endpoint is intended only for local debugging, consider disabling the route entirely outside local dev (not just hiding it from the schema), or at minimum remove/guard the fields that expose cookie contents in non-local environments.

Copilot uses AI. Check for mistakes.
Comment on lines +42 to 43
@router.get("/account/test.json", response_model=AuthTestResponse, tags=["internal"], include_in_schema=SHOW_INTERNAL_IN_SCHEMA)
async def check_authentication(
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

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

These three internal endpoints repeat the same tags=["internal"], include_in_schema=SHOW_INTERNAL_IN_SCHEMA kwargs. To reduce duplication and keep future changes consistent, consider factoring these into a shared constant (e.g., INTERNAL_ROUTE_KWARGS = {...}) or a small helper so the configuration only lives in one place.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

Please remove the infogami changes. I know they sneak in there at weird times.

running make git would probably fix it and then you can commit that.

Other than that it looks pretty good for me!

Thanks!

@RayBB RayBB self-assigned this Apr 2, 2026
@github-project-automation github-project-automation Bot moved this to Waiting Review/Merge from Staff in Ray's Project Apr 2, 2026
@Sadashii
Copy link
Copy Markdown
Contributor Author

Sadashii commented Apr 3, 2026

Fixed

Copy link
Copy Markdown
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

Great job!

@RayBB RayBB merged commit 22fd8a5 into internetarchive:master Apr 3, 2026
3 checks passed
@github-project-automation github-project-automation Bot moved this from Waiting Review/Merge from Staff to Done in Ray's Project Apr 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants