Skip to content

Conversation

@pcrespov
Copy link
Member

@pcrespov pcrespov commented Oct 1, 2025

Potential fix for https://github.com/ITISFoundation/osparc-simcore/security/code-scanning/95

To fix this problem, we should ensure that no sensitive credential data (e.g., API keys, secrets, tokens) is directly logged. Instead, log only identifying but non-sensitive information about the expired/removed API keys, such as their database IDs, associated user IDs, display names, or generic counts. Specifically, in services/web/server/src/simcore_service_webserver/garbage_collector/_tasks_api_keys.py, line 25 currently logs the variable api_key in full.

The fix involves:

  • Editing line 25 to avoid logging the full API key value.
  • Use available metadata if possible (such as a display name, if that's present in the list instead).
  • If only API key values are available, replace the full log with a generic message (e.g., logging just the count, or obfuscating part of the key—e.g., log a truncated or masked version).
  • If project context allows, logging the API key's display name or database ID is safe.

You will edit line 25 in _tasks_api_keys.py to something like:

_logger.info("Expired API key was removed")

Or, if safe and possible, log identifying metadata, e.g.:

_logger.info("API-key with display name '%s' expired and was removed", api_key.display_name)

But, as only the API key or name string is available, safest is to avoid logging the key string.

No imports or method definitions are required unless additional metadata needs fetching. Use only the shown code.


Suggested fixes powered by Copilot Autofix. Review carefully before merging.

…ensitive information

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@pcrespov pcrespov self-assigned this Oct 1, 2025
@pcrespov pcrespov added this to the Cheops milestone Oct 1, 2025
@codecov
Copy link

codecov bot commented Oct 1, 2025

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.58%. Comparing base (3d80890) to head (faac26b).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8442      +/-   ##
==========================================
+ Coverage   87.93%   89.58%   +1.65%     
==========================================
  Files        1976     1775     -201     
  Lines       77224    69756    -7468     
  Branches     1342      837     -505     
==========================================
- Hits        67904    62492    -5412     
+ Misses       8916     7041    -1875     
+ Partials      404      223     -181     
Flag Coverage Δ
integrationtests 64.26% <0.00%> (+0.08%) ⬆️
unittests 88.13% <0.00%> (+1.51%) ⬆️
Components Coverage Δ
pkg_aws_library 93.59% <ø> (ø)
pkg_celery_library 83.41% <ø> (ø)
pkg_dask_task_models_library 79.33% <ø> (ø)
pkg_models_library 93.08% <ø> (ø)
pkg_notifications_library 85.20% <ø> (ø)
pkg_postgres_database 87.95% <ø> (ø)
pkg_service_integration 70.19% <ø> (ø)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library 90.19% <ø> (ø)
pkg_simcore_sdk 84.99% <ø> (ø)
agent 93.53% <ø> (ø)
api_server 91.94% <ø> (ø)
autoscaling 95.74% <ø> (ø)
catalog 92.36% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.82% <ø> (-0.56%) ⬇️
datcore_adapter 97.94% <ø> (ø)
director 75.90% <ø> (+0.08%) ⬆️
director_v2 90.90% <ø> (-0.06%) ⬇️
dynamic_scheduler 96.68% <ø> (-0.09%) ⬇️
dynamic_sidecar 90.43% <ø> (ø)
efs_guardian 89.62% <ø> (ø)
invitations 91.44% <ø> (ø)
payments 92.62% <ø> (ø)
resource_usage_tracker 92.13% <ø> (ø)
storage 86.75% <ø> (+0.29%) ⬆️
webclient ∅ <ø> (∅)
webserver 87.72% <0.00%> (+0.03%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d80890...faac26b. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mergify
Copy link
Contributor

mergify bot commented Oct 1, 2025

🧪 CI Insights

Here's what we observed from your CI run for a0376e4.

❌ Job Failures

Pipeline Job Health on master Retries 🔍 CI Insights 📄 Logs
CI unit-tests Healthy 0 View View

✅ Passed Jobs With Interesting Signals

Pipeline Job Signal Health on master Retries 🔍 CI Insights 📄 Logs
CI system-tests Base branch is broken, but the job passed. Looks like this might be a real fix 💪 Broken 0 View View

@pcrespov pcrespov marked this pull request as ready for review October 3, 2025 09:33
@pcrespov pcrespov added the a:webserver webserver's codebase. Assigning the area is particularly useful for bugs label Oct 3, 2025
@pcrespov pcrespov changed the title Potential fix for code scanning alert no. 95: Clear-text logging of sensitive information 🔒️ Potential fix for code scanning alert no. 95: Clear-text logging of sensitive information Oct 3, 2025
@pcrespov pcrespov enabled auto-merge (squash) October 3, 2025 09:34
@pcrespov pcrespov added the security Pull requests that address a security vulnerability label Oct 3, 2025
@pcrespov pcrespov disabled auto-merge October 3, 2025 11:33
@pcrespov pcrespov merged commit 27ff515 into master Oct 3, 2025
75 of 80 checks passed
@pcrespov pcrespov deleted the alert-autofix-95 branch October 3, 2025 11:33
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 3, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a:webserver webserver's codebase. Assigning the area is particularly useful for bugs security Pull requests that address a security vulnerability

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants