Skip to content

Security hardening: remove committed session tokens, fix open redirect and debug server, validate input - #5

Open
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1787719498-security-hardening
Open

Security hardening: remove committed session tokens, fix open redirect and debug server, validate input#5
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
devin/1787719498-security-hardening

Conversation

@devin-ai-integration

Copy link
Copy Markdown

Summary

Security scan of the backend, frontend, git history, and dependencies. This PR fixes the code-level issues; two findings need action outside the repo (see below).

Highest-impact fixes

  • Committed OAuth tokens removed. flask_session/ (13 files) and flask_session_data/ (2 files) were tracked and several contained live access_token / refresh_token / spotify_user_id values. Untracked here and added to .gitignore (plus .env.*, *.db). The blobs still exist in git history, so app access should be revoked in Spotify.

  • Debugger no longer on by default. run.py ran app.run(host='0.0.0.0', debug=True); the Werkzeug debugger is an RCE console. Now FLASK_DEBUG (default off), FLASK_RUN_HOST (default 127.0.0.1), FLASK_RUN_PORT.

  • Open redirect in the OAuth flow. /login?redirect=https://evil.example/x was stored in the session and used verbatim as the post-/callback redirect. Now allowlisted:

    # config.py
    Config.safe_redirect(url)  # returns url only if f"{scheme}://{netloc}" in ALLOWED_REDIRECT_ORIGINS
                               # else DEFAULT_FRONTEND_REDIRECT

    Applied both when storing (/login) and when consuming (/callback), and the OAuth state is now compared with secrets.compare_digest and popped after use.

  • FLASK_SECRET_KEY fallback. SECRET_KEY = os.getenv(...) or secrets.token_hex(32) meant a production deploy without the env var silently got a per-process random key. Now raises when FLASK_ENV=production; ephemeral in dev only.

  • Expired tokens were used, never refreshed. Playlist routes only checked 'access_token' in session. Added a shared helper used by every authenticated route:

    # routes/auth.py
    def get_valid_access_token():  # refreshes when past expires_at, clears session and returns None on failure
  • Input validation on playlist routes (routes/playlist.py): playlistLength clamped to 1..MAX_PLAYLIST_LENGTH (was int(user_value), so 10**9 drove AI + Spotify request volume), uris must match ^spotify:track:[A-Za-z0-9]{22}$ and number ≤ 100, cover image must be valid base64 ≤ 256KB, q/name/description truncated, and a global MAX_CONTENT_LENGTH of 2MB.

  • Bug fix that was also a 500-generator: /Search_Track and /Get_Playlists called requests.get(...) with no import requests — every hit raised NameError and returned the exception string to the client.

  • Error leakage: str(e) and raw Spotify response bodies are no longer returned; they go to logger.exception() and clients get a generic message with a 502.

  • CORS origin list moved to the CORS_ORIGINS env var (defaults unchanged), and outbound Spotify calls got 10–15s timeouts.

Dependencies: requests 2.32.3→2.32.5, urllib3 2.3.0→2.5.0; npm audit fix (lockfile only, no package.json changes) takes the frontend from 23 advisories (17 high) to 5.

Needs your action (not fixable in this PR)

  1. .env with the real CLIENT_ID/CLIENT_SECRET was committed in 40e3f47 and deleted in 000c2cd — still readable from history. Rotate the Spotify client secret (and GENAI_API_KEY/FLASK_SECRET_KEY if ever committed).
  2. Revoke the app's Spotify access so the refresh tokens that were in flask_session/ are dead.

Not fixed, deliberately

  • No CSRF protection on cookie-authenticated POSTs; production config uses SameSite=None, which removes the implicit protection. Worth adding before a public deploy.
  • Remaining 5 npm advisories need breaking bumps (vite 5→6, react-router-dom 6→7) — separate PR.
  • legacy/ still has debug=True, a 'dev-secret-key' fallback, and debug prints of session dicts / token prefixes. It is dead code (not imported by backend/); deleting the directory is the cleanest fix.
  • No SQL injection found (ORM-only, no raw SQL/eval/shell) and no exposed debug endpoints in backend/.

Testing

app.test_client() smoke test: unauthenticated /auth/status, /Create_Playlist, /Search_Track, /Playlist_Generator all 401; /login?redirect=http://evil.com/steal redirects to Spotify with the session redirect coerced to the default; validation helpers reject oversized lengths, malformed URIs, and non-base64/oversized images. Note: npm run build fails on main too — src/lib/utils was never committed — so the frontend build is unrelated pre-existing breakage.

Link to Devin session: https://app.devin.ai/sessions/95fd45ae26db4cdcae838efb27e2dbef
Requested by: @praneels2005

@devin-ai-integration

Copy link
Copy Markdown
Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

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.

1 participant