Skip to content

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Oct 16, 2025

What do these changes do?

This pull request introduces a new module, string_types.py, which centralizes and enforces stricter validation for string types used in API schemas. The main goal is to improve input safety and consistency across models by replacing generic string fields with custom, strongly validated types (such as NameSafeStr, DescriptionSafeStr, and others).

This change is applied to several (but not all) API schemas related to folders, groups, projects, and users, and removes legacy string type definitions from basic_types.py.

Input Safety & String Type Improvements

  • Introduced string_types.py with new validated types: NameSafeStr, DescriptionSafeStr, GlobPatternSafeStr, ShortTruncatedStr, and LongTruncatedStr, all enforcing stricter content and length rules, and protecting against SQL/JS injection.
  • Updated API schema fields in folders_v2.py, groups.py, projects.py, and users.py to use the new safe string types instead of plain str or legacy types, improving input validation and safety for user-provided data.

Follow up

NOTE that this is just an initial PR that set the bases and modifies some inputs as examples

  • MUST be applied to all INPUT fields (queries, bodies, path etc) to web- and public- APIs
  • (optionally) Front-end can account for validation error codes (see test_safe_string_types) in the response error body and complements 422 (unprocessable entyty) or 400 (Bad request) status codes @odeimaiz

Related issue/s

  • Discussions about API inputs during PM2 meeting

How to test

cd packages/models-library
make install-dev
pytest -vv tests/test_string_types.py 
  • Removed outdated tests for legacy string types from test_basic_types.py, as validation is now handled by the new types in string_types.py.

Dev-ops

None

@pcrespov pcrespov self-assigned this Oct 16, 2025
@pcrespov pcrespov added a:api framework api, data schemas, t:maintenance Some planned maintenance work a:models-library labels Oct 16, 2025
Copy link

codecov bot commented Oct 16, 2025

Codecov Report

❌ Patch coverage is 94.28571% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.49%. Comparing base (06eca11) to head (f75cc46).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8526      +/-   ##
==========================================
+ Coverage   89.32%   89.49%   +0.16%     
==========================================
  Files        1801     1584     -217     
  Lines       70811    65593    -5218     
  Branches      837      504     -333     
==========================================
- Hits        63252    58702    -4550     
+ Misses       7339     6759     -580     
+ Partials      220      132      -88     
Flag Coverage Δ
integrationtests 63.89% <100.00%> (-0.07%) ⬇️
unittests 87.95% <94.28%> (+0.05%) ⬆️
Components Coverage Δ
pkg_aws_library ∅ <ø> (∅)
pkg_celery_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library 92.88% <93.54%> (-0.02%) ⬇️
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration 70.17% <ø> (ø)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 84.95% <ø> (ø)
agent 93.10% <ø> (ø)
api_server 91.62% <100.00%> (ø)
autoscaling 95.00% <ø> (ø)
catalog 92.06% <ø> (ø)
clusters_keeper 99.14% <ø> (ø)
dask_sidecar 92.38% <ø> (+0.55%) ⬆️
datcore_adapter 97.95% <ø> (ø)
director 75.72% <ø> (ø)
director_v2 90.87% <ø> (-0.10%) ⬇️
dynamic_scheduler 96.82% <ø> (ø)
dynamic_sidecar 90.44% <ø> (ø)
efs_guardian 89.83% <ø> (ø)
invitations 90.90% <ø> (ø)
payments 92.80% <ø> (ø)
resource_usage_tracker 92.16% <ø> (ø)
storage 86.56% <ø> (-0.37%) ⬇️
webclient ∅ <ø> (∅)
webserver 87.10% <100.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 06eca11...f75cc46. 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.

@pcrespov pcrespov force-pushed the mai/description-type branch from ac96308 to 153107f Compare October 16, 2025 17:45
@pcrespov pcrespov added the 🤖-automerge marks PR as ready to be merged for Mergify label Oct 16, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a new string_types.py module that centralizes validation for string fields used in API schemas, replacing generic str types with strongly validated custom types (NameSafeStr, DescriptionSafeStr, etc.) that enforce content safety rules and length constraints to protect against SQL/JS injection attacks.

Key Changes:

  • Created string_types.py with validated string types that detect SQL/JS injection patterns
  • Applied new safe string types to API input schemas for folders, groups, projects, tags, and users
  • Migrated truncated string types from basic_types.py to string_types.py with added safety validation

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
packages/models-library/src/models_library/string_types.py New module defining validated string types with injection protection
packages/models-library/src/models_library/basic_types.py Removed legacy truncated string type definitions
packages/models-library/src/models_library/api_schemas_webserver/folders_v2.py Updated folder name fields to use NameSafeStr
packages/models-library/src/models_library/api_schemas_webserver/groups.py Updated group label/description fields to use safe string types
packages/models-library/src/models_library/api_schemas_webserver/users.py Updated user profile and search fields to use safe string types
packages/models-library/src/models_library/api_schemas_webserver/projects.py Updated import path for truncated string types
packages/models-library/src/models_library/users.py Added UserNameSafeID type with input safety validation
services/web/server/src/simcore_service_webserver/tags/schemas.py Updated tag schemas to use safe string types and moved ColorStr definition
services/api-server/src/simcore_service_api_server/api/routes/studies.py Updated import path for truncated string types
services/web/server/src/simcore_service_webserver/api/v0/openapi.yaml Added pattern constraints to OpenAPI schema definitions
packages/models-library/tests/test_string_types.py New test file for safe string type validation
packages/models-library/tests/test_basic_types.py Removed tests for legacy truncated string types
services/web/server/tests/unit/with_dbs/03/users/conftest.py Moved imports to module level

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@pcrespov
Copy link
Member Author

@mergify queue

Copy link
Contributor

mergify bot commented Oct 16, 2025

queue

🛑 Configuration not compatible with a branch protection setting

The branch protection setting Require branches to be up to date before merging is not compatible with draft PR checks. To keep this branch protection enabled, update your Mergify configuration to enable in-place checks: set merge_queue.max_parallel_checks: 1, set every queue rule batch_size: 1, and avoid two-step CI (make merge_conditions identical to queue_conditions). Otherwise, disable this branch protection.

@pcrespov pcrespov added this to the Imparable milestone Oct 16, 2025
@pcrespov pcrespov enabled auto-merge (squash) October 16, 2025 18:02
Copy link
Contributor

mergify bot commented Oct 16, 2025

🧪 CI Insights

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

🟢 All jobs passed!

But CI Insights is watching 👀

Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

I like this PR, but I have one concern with it, which is existing data.

I'd like to avoid issue on production deployments with this PR.

Can you please import and check that running these validators on all the production data does not cause any errors?

Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

Please double check if these mega regexes are not subject to backtracking issues

@YuryHrytsuk
Copy link
Contributor

👀

@pcrespov pcrespov force-pushed the mai/description-type branch from 8de641f to 9f0d5fd Compare October 17, 2025 13:48
Copy link
Member

@odeimaiz odeimaiz left a comment

Choose a reason for hiding this comment

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

I'll follow up, thanks!

…eStr in folder and group schemas; enhance OpenAPI schema with validation patterns for name and description fields.
… and LongTruncatedStr to emphasize silent handling of large inputs; refactor import statement in test_projects.py for consistency.
…improved security; add SearchPatternSafeStr type for flexible pattern matching.
@pcrespov pcrespov force-pushed the mai/description-type branch from ef1d88a to dd3ca38 Compare October 21, 2025 14:38
@pcrespov pcrespov disabled auto-merge October 21, 2025 16:04
Copy link

@pcrespov
Copy link
Member Author

@mergify queue

Copy link
Contributor

mergify bot commented Oct 21, 2025

queue

🛑 Configuration not compatible with a branch protection setting

The branch protection setting Require branches to be up to date before merging is not compatible with draft PR checks. To keep this branch protection enabled, update your Mergify configuration to enable in-place checks: set merge_queue.max_parallel_checks: 1, set every queue rule batch_size: 1, and avoid two-step CI (make merge_conditions identical to queue_conditions). Otherwise, disable this branch protection.

@pcrespov pcrespov merged commit 48c7ccc into ITISFoundation:master Oct 21, 2025
94 of 95 checks passed
@pcrespov pcrespov deleted the mai/description-type branch October 21, 2025 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:api framework api, data schemas, a:models-library t:maintenance Some planned maintenance work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants