Skip to content

fix: store session_id#163

Merged
cbachhuber merged 1 commit intomainfrom
161-check-ios-safari-on-streamlitcom
Mar 29, 2026
Merged

fix: store session_id#163
cbachhuber merged 1 commit intomainfrom
161-check-ios-safari-on-streamlitcom

Conversation

@BayerC
Copy link
Copy Markdown
Owner

@BayerC BayerC commented Mar 28, 2026

This will (hopefully) solve the issue.
The problem is we need to merge it to see if it works.

Also its maybe suboptimal to expose the session id ...
I would be open to better alternatives

@BayerC BayerC requested a review from cbachhuber March 28, 2026 11:53
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 issue

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="src/open_cups/session_state.py" line_range="15-16" />
<code_context>
     def __init__(self) -> None:
         if "session_id" not in st.session_state:
-            st.session_state.session_id = str(uuid.uuid4())
+            existing = st.query_params.get("session_id")
+            st.session_state.session_id = existing or str(uuid.uuid4())
+        st.query_params["session_id"] = st.session_state.session_id

</code_context>
<issue_to_address>
**🚨 issue (security):** Using a raw query param as the session_id risks session fixation or ID spoofing.

Since `session_id` can be fully controlled via the URL, avoid treating it as an authoritative session identifier unless it’s strongly validated or protected. Either validate and constrain the value (e.g., format/length, signed token) or only use it to look up/attach to an existing server-side session instead of trusting an arbitrary ID from the query string.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +15 to +16
existing = st.query_params.get("session_id")
st.session_state.session_id = existing or str(uuid.uuid4())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚨 issue (security): Using a raw query param as the session_id risks session fixation or ID spoofing.

Since session_id can be fully controlled via the URL, avoid treating it as an authoritative session identifier unless it’s strongly validated or protected. Either validate and constrain the value (e.g., format/length, signed token) or only use it to look up/attach to an existing server-side session instead of trusting an arbitrary ID from the query string.

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (787541c) to head (498c9f7).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #163   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          527       529    +2     
=========================================
+ Hits           527       529    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Collaborator

@cbachhuber cbachhuber left a comment

Choose a reason for hiding this comment

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

Thanks for investigating! Let's see if this fixes it

@cbachhuber cbachhuber merged commit a8e9a51 into main Mar 29, 2026
9 checks passed
@cbachhuber
Copy link
Copy Markdown
Collaborator

This fixes it for me! ❤️

tested on streamlit.com with iOS and Brave browser

image

Can you confirm, @BayerC?

@BayerC
Copy link
Copy Markdown
Owner Author

BayerC commented Mar 30, 2026

yes it works but now copy pasting the url link is buggy :/

@cbachhuber
Copy link
Copy Markdown
Collaborator

@BayerC what exactly is buggy? Should we have a new issue for that?

@BayerC
Copy link
Copy Markdown
Owner Author

BayerC commented Mar 31, 2026

The problem is if xou copy paste the link to give it to a collegue or open a new tab, you will not be a news session since the session id is in the url sou two tabs are logged in as same user.

Yes please create issue and test it by xouself

@cbachhuber
Copy link
Copy Markdown
Collaborator

Created #165

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.

2 participants