fix(config): expose build details (git SHA/build number) to admins only#41301
fix(config): expose build details (git SHA/build number) to admins only#41301rusackas wants to merge 1 commit into
Conversation
VERSION_SHA and BUILD_NUMBER were included in the "About" section and the bootstrap payload for all viewers, letting anyone able to load the page map the deployment to an exact commit/build. Gate these precise build details behind a new EXPOSE_BUILD_DETAILS_TO_USERS config (env: SUPERSET_EXPOSE_BUILD_DETAILS, default off): they are included only for admins unless the deployment opts in. The release version string is still shown to everyone. Adds a pure visible_version_metadata() helper with unit tests and documents the change in UPDATING.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dosubot
Bot
added
authentication:RBAC
Code Review Agent Run #6a880dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| from superset.utils.version import visible_version_metadata | ||
|
|
||
|
|
||
| def _metadata(): |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to this helper function so the newly added code is fully typed. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The file is newly added Python code, and this helper function has no type hints for its return value. That matches the rule requiring newly added Python functions to be fully typed.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/version_test.py
**Line:** 22:22
**Comment:**
*Custom Rule: Add an explicit return type annotation to this helper function so the newly added code is fully typed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| } | ||
|
|
||
|
|
||
| def test_visible_version_metadata_hides_build_details_when_not_exposed(): |
There was a problem hiding this comment.
Suggestion: Add a -> None return annotation to this new test function to satisfy full typing requirements for added Python functions. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The function is newly added Python code and it lacks a return type annotation. That is a real violation of the full-typing rule for new functions.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/version_test.py
**Line:** 30:30
**Comment:**
*Custom Rule: Add a `-> None` return annotation to this new test function to satisfy full typing requirements for added Python functions.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| assert result["build_number"] is None | ||
|
|
||
|
|
||
| def test_visible_version_metadata_keeps_build_details_when_exposed(): |
There was a problem hiding this comment.
Suggestion: Add a -> None return annotation to this new test function so the added code includes complete type hints. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This newly added test function has no return type annotation, which violates the rule that new Python functions should be fully typed.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/version_test.py
**Line:** 38:38
**Comment:**
*Custom Rule: Add a `-> None` return annotation to this new test function so the added code includes complete type hints.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| assert result == metadata | ||
|
|
||
|
|
||
| def test_visible_version_metadata_does_not_mutate_input(): |
There was a problem hiding this comment.
Suggestion: Add a -> None return annotation to this new test function to align with the rule requiring full typing in newly changed code. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This newly introduced test function also lacks a return type annotation, so it fits the typing rule violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/utils/version_test.py
**Line:** 45:45
**Comment:**
*Custom Rule: Add a `-> None` return annotation to this new test function to align with the rule requiring full typing in newly changed code.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41301 +/- ##
==========================================
- Coverage 64.34% 64.34% -0.01%
==========================================
Files 2653 2653
Lines 144952 144958 +6
Branches 33433 33434 +1
==========================================
+ Hits 93273 93274 +1
- Misses 49995 49998 +3
- Partials 1684 1686 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| """ | ||
| if expose_build_details: | ||
| return metadata | ||
| return {**metadata, "version_sha": "", "build_number": None} |
There was a problem hiding this comment.
Suggestion: The redaction helper does not remove full_sha, even though that field is an exact git commit identifier and is part of the metadata produced upstream. With expose_build_details=False, callers that serialize this returned dict can still leak precise build identity. Redact/remove full_sha together with the short SHA/build number. [security]
Severity Level: Major ⚠️
- ⚠️ Redaction helper leaves full_sha populated when hiding details.
- ⚠️ Future serializers may expose exact commit SHA to users.Steps of Reproduction ✅
1. Configure Superset with `EXPOSE_BUILD_DETAILS_TO_USERS = False` in
`superset/config.py:54-61` and run the server in an environment where `GITHUB_SHA` is set
(e.g. CI-built container), so `get_version_metadata()` will populate `full_sha`.
2. Trigger any code path that calls `menu_data(user)`; for example, a SPA page render via
`SupersetModelView.render_app_template()` in `superset/views/base.py:670-672`, which calls
`get_spa_template_context()` (`base.py:583-587`), then `common_bootstrap_payload()`
(`base.py:38-42`), then `cached_common_bootstrap_data()` (`base.py:452-456`), which builds
`"menu_data": menu_data(g.user)` at `base.py:528-528`.
3. Inside `menu_data()` (`superset/views/base.py:273-291`), `version_metadata =
visible_version_metadata(get_version_metadata(), expose_build_details)` is executed
(`base.py:289-291`). `get_version_metadata()` from `superset/utils/version.py:29-57` reads
`GITHUB_SHA` and sets `metadata["full_sha"] = github_sha` at `version.py:44`.
4. Because `expose_build_details` is False for a non-admin user,
`visible_version_metadata()` in `superset/utils/version.py:60-72` returns `{**metadata,
"version_sha": "", "build_number": None}` (`version.py:72`), leaving
`metadata["full_sha"]` untouched. Inspecting `version_metadata` in a debugger or via
temporary logging confirms that `version_metadata["full_sha"]` still contains the exact
commit SHA even though build details are supposed to be hidden, so any caller that later
serializes the whole dict (instead of selecting keys as `menu_data` currently does) would
leak the full commit identifier.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/utils/version.py
**Line:** 72:72
**Comment:**
*Security: The redaction helper does not remove `full_sha`, even though that field is an exact git commit identifier and is part of the metadata produced upstream. With `expose_build_details=False`, callers that serialize this returned dict can still leak precise build identity. Redact/remove `full_sha` together with the short SHA/build number.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| version_metadata = visible_version_metadata( | ||
| get_version_metadata(), expose_build_details | ||
| ) |
There was a problem hiding this comment.
Suggestion: menu_data now calls get_version_metadata() on the bootstrap path, and that helper can run git rev-parse subprocess calls when CI env vars are not present. Since this code is reached for normal page bootstrap traffic, it introduces avoidable per-request process-spawn overhead (memoized only for 60s per user/locale). Use already-loaded config values for navbar version fields or memoize version metadata independently of request/user context. [performance]
Severity Level: Major ⚠️
- ⚠️ spa.html bootstrap path runs git subprocess on cache misses.
- ⚠️ Superset UI requests incur extra CPU and process overhead.Steps of Reproduction ✅
1. Run Superset in an environment where `GITHUB_HEAD_REF` and `GITHUB_REF_NAME` are unset
(typical non-CI deployment) so `get_version_metadata()` must fall back to
`_get_local_branch()`; confirm no branch env vars are set.
2. From a browser, load any SPA page rendered via
`SupersetModelView.render_app_template()` in `superset/views/base.py:670-672`, which calls
`get_spa_template_context()` (`base.py:583-587`), then `common_bootstrap_payload()`
(`base.py:38-42`), which in turn calls `cached_common_bootstrap_data(utils.get_user_id(),
locale_str)` at `base.py:42`.
3. On a cache miss for `cached_common_bootstrap_data` (per
`@cache_manager.cache.memoize(timeout=60)` at `superset/views/base.py:452-456`), the
function constructs the bootstrap dict including `"menu_data": menu_data(g.user)` at
`base.py:528-528`. Inside `menu_data()` (`base.py:273-291`), it computes `version_metadata
= visible_version_metadata(get_version_metadata(), expose_build_details)` at
`base.py:289-291`.
4. `get_version_metadata()` (`superset/utils/version.py:29-57`) calls
`_get_local_branch()` at `version.py:48-53`, which executes
`subprocess.check_output(["git", "rev-parse", "--abbrev-ref", "HEAD"], ...)` in
`_get_local_branch()` (`version.py:106-115`). Profiling or adding temporary logging around
`_get_local_branch()` shows a `git rev-parse` subprocess being spawned on each cache miss
(i.e., at least once every 60 seconds per user/locale) along this common bootstrap path.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/views/base.py
**Line:** 289:291
**Comment:**
*Performance: `menu_data` now calls `get_version_metadata()` on the bootstrap path, and that helper can run `git rev-parse` subprocess calls when CI env vars are not present. Since this code is reached for normal page bootstrap traffic, it introduces avoidable per-request process-spawn overhead (memoized only for 60s per user/locale). Use already-loaded config values for navbar version fields or memoize version metadata independently of request/user context.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix2 participants
SUMMARY
VERSION_SHAandBUILD_NUMBERwere surfaced in the frontend "About" section and the common bootstrap payload for every viewer, disclosing the precise git commit and build of the deployment (useful reconnaissance for mapping to known issues for an exact build).This gates those precise build details behind a new
EXPOSE_BUILD_DETAILS_TO_USERSconfig (env:SUPERSET_EXPOSE_BUILD_DETAILS, default off). The git SHA and build number are included only for admin users unless a deployment opts in. The releaseversion_stringis still shown to everyone (it is widely used in the UI/About).A small pure helper
visible_version_metadata()encapsulates the gating so it is easy to test.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — bootstrap payload contents.
TESTING INSTRUCTIONS
Unit tests added in
tests/unit_tests/utils/version_test.py:version_sha/build_numberare blanked whileversion_stringis kept.Run:
pytest tests/unit_tests/utils/version_test.pyManual: as a non-admin, the bootstrap payload
navbar_right.version_shais empty andbuild_numberis null; as an admin (or withSUPERSET_EXPOSE_BUILD_DETAILS=true) they are populated.ADDITIONAL INFORMATION