Add configurable legal links (Impressum, Privacy, Terms)#396
Merged
Conversation
Every page now shows Impressum, Privacy Policy and Terms of Use links in the sidebar footer, and the GDPR consent banner links to the privacy policy so consent and policy are tied together. Links default to the official OpenMS pages and are overridable via a new "legal_links" object in settings.json, so self-hosting forks can point them at their own legal pages (an Impressum must name the actual operator). - settings.json: add legal_links (impressum/privacy/terms) defaults - common.py: add get_legal_links() (settings overrides merged over the hardcoded OpenMS defaults, so older forks still inherit links) and render the three links in the sidebar on every page - captcha_.py: forward the privacy policy URL to the consent component - gdpr_consent: set Klaro privacyPolicyUrl so the banner shows a working privacy-policy link; rebuild dist/bundle.js - README: document the legal_links override - tests: cover get_legal_links default/override/fallback behavior https://claude.ai/code/session_01WCamMNCunp9T2aScEKKUvx
The module-level streamlit/dependency mocks were left in sys.modules after import, leaking into later-collected test modules and breaking the AppTest-based tests (test_run_subprocess, test_simple_workflow) with "ModuleNotFoundError: No module named 'streamlit.testing'; 'streamlit' is not a package". Save and restore the mocked sys.modules entries and drop the cached src.common.common module after import, mirroring the established pattern in tests/test_parameter_presets.py. Full suite now passes (39 passed, 2 skipped). https://claude.ai/code/session_01WCamMNCunp9T2aScEKKUvx
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds configurable legal page links (Impressum, Privacy Policy, Terms of Use) to the sidebar. A new ChangesLegal Links Feature
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/common/common.py (2)
798-810: 💤 Low valueConsider sanitizing URLs before HTML injection for defense in depth.
The legal link URLs from
get_legal_links()are directly interpolated into HTML using f-strings without sanitization or escaping (lines 805-807). Whilesettings.jsonis a trusted configuration file and not user input, sanitizing URLs would add defense in depth against accidental or malicious configuration changes.Consider using
html.escape()for the URLs or validating them against an allowed pattern.🛡️ Proposed fix to escape URLs
+import html + # Legal links (Impressum, Privacy Policy, Terms of Use), shown on every # page. URLs are configurable via "legal_links" in settings.json. links = get_legal_links() st.markdown( '<div style="text-align:center; font-size:0.8rem; ' 'margin-top:0.5rem; color:`#a4a5ad`;">' - f'<a href="{links["impressum"]}" target="_blank" rel="noopener">Impressum</a> · ' - f'<a href="{links["privacy"]}" target="_blank" rel="noopener">Privacy Policy</a> · ' - f'<a href="{links["terms"]}" target="_blank" rel="noopener">Terms of Use</a>' + f'<a href="{html.escape(links["impressum"])}" target="_blank" rel="noopener">Impressum</a> · ' + f'<a href="{html.escape(links["privacy"])}" target="_blank" rel="noopener">Privacy Policy</a> · ' + f'<a href="{html.escape(links["terms"])}" target="_blank" rel="noopener">Terms of Use</a>' "</div>", unsafe_allow_html=True, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/common.py` around lines 798 - 810, The URLs obtained from get_legal_links() are directly interpolated into HTML markup without escaping, creating a potential security risk if the configuration file is ever compromised. Wrap each URL reference (links["impressum"], links["privacy"], and links["terms"]) with html.escape() before including them in the f-strings to sanitize the URLs and prevent HTML injection attacks even when using unsafe_allow_html=True with st.markdown().
553-566: Remove the unconditionalcaptcha_control()call at line 553.The function
captcha_control()is called unconditionally at line 553 and again conditionally at line 566 with identical arguments. Sincecaptcha_control()internally guards its logic with the same condition (if "controllo" not in st.session_state or st.session_state["controllo"] == False), the first unconditional call is redundant. The function will either display the CAPTCHA/consent flow on both calls or skip its main logic on both—making the unconditional call unnecessary. Removing line 553 and the blank line after it simplifies the flow without changing behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/common.py` around lines 553 - 566, Remove the redundant unconditional call to captcha_control(privacy_policy_url=get_legal_links()["privacy"]) that appears before the conditional block. Since the same captcha_control function is called again immediately after within an if statement that checks for "controllo" not in st.session_state or the condition on params["controllo"], and since captcha_control internally guards its logic with the same condition, the first unconditional call is duplicate and should be deleted along with any associated blank lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/common/common.py`:
- Around line 798-810: The URLs obtained from get_legal_links() are directly
interpolated into HTML markup without escaping, creating a potential security
risk if the configuration file is ever compromised. Wrap each URL reference
(links["impressum"], links["privacy"], and links["terms"]) with html.escape()
before including them in the f-strings to sanitize the URLs and prevent HTML
injection attacks even when using unsafe_allow_html=True with st.markdown().
- Around line 553-566: Remove the redundant unconditional call to
captcha_control(privacy_policy_url=get_legal_links()["privacy"]) that appears
before the conditional block. Since the same captcha_control function is called
again immediately after within an if statement that checks for "controllo" not
in st.session_state or the condition on params["controllo"], and since
captcha_control internally guards its logic with the same condition, the first
unconditional call is duplicate and should be deleted along with any associated
blank lines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 761755f5-ba6e-4949-af99-6d1cfe4c14c1
⛔ Files ignored due to path filters (1)
gdpr_consent/dist/bundle.jsis excluded by!**/dist/**
📒 Files selected for processing (6)
README.mdgdpr_consent/src/main.tssettings.jsonsrc/common/captcha_.pysrc/common/common.pytests/test_legal_links.py
The streamlit mock was installed into sys.modules, but src.common.common was only popped AFTER importing get_legal_links. When test_gui.py runs first (CI order: `pytest test_gui.py tests/`), it imports the real, streamlit-bound common module into sys.modules; the subsequent mock-based import here was then a cache hit returning the real-streamlit-bound function, so the tests' session_state overrides had no effect and get_legal_links returned the OpenMS defaults. The two override tests failed in CI (they passed locally only when collected first). Pop src.common.common BEFORE the import to force a fresh import under the mock, then restore the original module afterward so the AppTest-based test modules still get the genuine package. Full suite: 76 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WCamMNCunp9T2aScEKKUvx
In the narrow sidebar, the footer links broke at the space inside a label (e.g. "Terms of Use" split into "Terms" / "of Use"). Add white-space:nowrap to each link so labels stay intact; line breaks now happen only between links. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WCamMNCunp9T2aScEKKUvx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds support for configurable legal page URLs (Impressum, Privacy Policy, Terms of Use) that appear in the sidebar footer on every page and in the GDPR consent banner. Self-hosted forks can now override these links via
settings.jsonwhile maintaining sensible OpenMS defaults for apps built from older configurations.Key Changes
get_legal_links()function insrc/common/common.pythat merges optionallegal_linksoverrides fromsettings.jsonwith built-in OpenMS defaults, ensuring empty/blank overrides don't erase defaultsDEFAULT_LEGAL_LINKSconstant pointing to official OpenMS pages (https://openms.de/impressum,/privacy,/terms), allowing downstream apps without alegal_linkskey to inherit working links by defaultpage_setup()displays the three legal links with proper styling andtarget="_blank"behaviorget_legal_links()is now passed to the consent component viacaptcha_control(privacy_policy_url=...), keeping consent and policy in syncgdpr_consent/src/main.ts) to accept and wire the privacy policy URL into Klaro's configurationsettings.jsonconfigurationtests/test_legal_links.py) covering defaults, missing settings, full overrides, partial overrides, and empty/None value handling, with proper mocking to avoid Streamlit runtime dependenciesImplementation Details
{**DEFAULT_LEGAL_LINKS, **{k: v for k, v in overrides.items() if v}}to ensure falsy override values (empty strings,None) fall back to defaults rather than erasing themFakeSessionStateclass to mock Streamlit's session state without requiring a running app context, mirroring the pattern intests/test_parameter_presets.pysettings.jsonexample includes the full default configuration for clarity, though the code gracefully handles missing or partiallegal_linksobjectshttps://claude.ai/code/session_01WCamMNCunp9T2aScEKKUvx
Summary by CodeRabbit
New Features
Documentation