From 0b1eca1a7a72292c3324e055a8bb8acb76463f8b Mon Sep 17 00:00:00 2001 From: felix Date: Wed, 3 Dec 2025 12:54:20 -0500 Subject: [PATCH 1/5] add rubocop for Gitlab::Json.safe_parse --- .rubocop.yml | 9 + .rubocop_todo/gitlab/json_safe_parse.yml | 525 ++++++++++++++++++ .../secure_coding_guidelines/_index.md | 85 +++ rubocop/cop/gitlab/json_safe_parse.rb | 73 +++ .../cop/gitlab/json_safe_parse_spec.rb | 152 +++++ 5 files changed, 844 insertions(+) create mode 100644 .rubocop_todo/gitlab/json_safe_parse.yml create mode 100644 rubocop/cop/gitlab/json_safe_parse.rb create mode 100644 spec/rubocop/cop/gitlab/json_safe_parse_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index 5efe069b36cc9a..ee9363dbc61958 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -460,6 +460,15 @@ Gitlab/Json: - 'lib/quality/**/*' - 'tooling/danger/**/*' +Gitlab/JsonSafeParse: + Description: 'Prefer Gitlab::Json.safe_parse over Gitlab::Json.parse for untrusted input' + Enabled: true + Exclude: + - 'qa/**/*' + - 'scripts/**/*' + - 'lib/quality/**/*' + - 'tooling/danger/**/*' + Gitlab/AvoidUploadedFileFromParams: Enabled: true Exclude: diff --git a/.rubocop_todo/gitlab/json_safe_parse.yml b/.rubocop_todo/gitlab/json_safe_parse.yml new file mode 100644 index 00000000000000..7126e81e6ee6f0 --- /dev/null +++ b/.rubocop_todo/gitlab/json_safe_parse.yml @@ -0,0 +1,525 @@ +--- +# Cop supports --autocorrect. +Gitlab/JsonSafeParse: + Details: grace period + Exclude: + - 'app/controllers/admin/database_diagnostics_controller.rb' + - 'app/controllers/event_forward/event_forward_controller.rb' + - 'app/graphql/resolvers/package_pipelines_resolver.rb' + - 'app/helpers/icons_helper.rb' + - 'app/models/active_session.rb' + - 'app/models/blob_viewer/dependency_manager.rb' + - 'app/models/bulk_imports/export_status.rb' + - 'app/models/ci/pipeline_creation/requests.rb' + - 'app/models/clusters/platforms/kubernetes.rb' + - 'app/models/concerns/diff_positionable_note.rb' + - 'app/models/concerns/integrations/base/mock_ci.rb' + - 'app/models/concerns/integrations/base/mock_monitoring.rb' + - 'app/models/concerns/redis_cacheable.rb' + - 'app/models/import/source_user_placeholder_reference.rb' + - 'app/presenters/ci/pipeline_artifacts/code_coverage_presenter.rb' + - 'app/presenters/ci/pipeline_artifacts/code_quality_mr_diff_presenter.rb' + - 'app/services/activity_pub/inbox_resolver_service.rb' + - 'app/services/audit_events/processor.rb' + - 'app/services/authn/passkey/authenticate_service.rb' + - 'app/services/authn/passkey/register_service.rb' + - 'app/services/ci/parse_annotations_artifact_service.rb' + - 'app/services/dependency_proxy/request_token_service.rb' + - 'app/services/ide/schemas_config_service.rb' + - 'app/services/jira/requests/base.rb' + - 'app/services/mcp/tools/api_service.rb' + - 'app/services/mcp/tools/api_tool.rb' + - 'app/services/packages/composer/composer_json_service.rb' + - 'app/services/packages/npm/check_manifest_coherence_service.rb' + - 'app/services/projects/lfs_pointers/lfs_download_link_list_service.rb' + - 'app/services/security/ci_configuration/sast_parser_service.rb' + - 'app/services/web_hook_service.rb' + - 'app/services/webauthn/authenticate_service.rb' + - 'app/validators/json_schema_validator.rb' + - 'app/workers/gitlab/version/version_check_cron_worker.rb' + - 'ee/app/graphql/resolvers/sbom/dependency_paths_resolver.rb' + - 'ee/app/graphql/resolvers/vulnerabilities_resolver.rb' + - 'ee/app/graphql/types/compliance_management/compliance_requirement_control_type.rb' + - 'ee/app/graphql/types/json_string_type.rb' + - 'ee/app/helpers/ee/organizations/organization_helper.rb' + - 'ee/app/models/ai/conversation/message.rb' + - 'ee/app/models/compliance_management/compliance_framework/compliance_requirements_control.rb' + - 'ee/app/models/compliance_management/control_expression.rb' + - 'ee/app/models/concerns/audit_events/externally_streamable.rb' + - 'ee/app/models/ee/application_setting.rb' + - 'ee/app/models/ee/ci/pipeline.rb' + - 'ee/app/models/preloaders/user_member_roles_in_groups_preloader.rb' + - 'ee/app/models/preloaders/user_member_roles_in_projects_preloader.rb' + - 'ee/app/models/secrets_management/audit_log.rb' + - 'ee/app/models/security/remediations_proxy.rb' + - 'ee/app/models/virtual_registries/container/upstream.rb' + - 'ee/app/models/vulnerabilities/finding.rb' + - 'ee/app/services/ai/duo_workflows/start_workflow_service.rb' + - 'ee/app/services/alert_management/extract_alert_payload_fields_service.rb' + - 'ee/app/services/arkose/status_service.rb' + - 'ee/app/services/geo/base_batch_bulk_update_service.rb' + - 'ee/app/services/geo/container_repository_sync.rb' + - 'ee/app/services/gitlab_subscriptions/fetch_subscription_plans_service.rb' + - 'ee/app/services/product_analytics/cube_data_query_service.rb' + - 'ee/app/services/sbom/ingestion/vulnerability_data.rb' + - 'ee/app/services/search/zoekt/provisioning_service.rb' + - 'ee/app/services/search/zoekt/rollout_service.rb' + - 'ee/app/services/security/process_scan_events_service.rb' + - 'ee/app/services/security/token_revocation_service.rb' + - 'ee/app/workers/analytics/value_stream_dashboard/count_worker.rb' + - 'ee/app/workers/click_house/rebuild_materialized_view_cron_worker.rb' + - 'ee/app/workers/observability/alert_query_worker.rb' + - 'ee/app/workers/product_analytics/initialize_snowplow_product_analytics_worker.rb' + - 'ee/lib/ai/context/dependencies/config_files/cpp_vcpkg.rb' + - 'ee/lib/ai/context/dependencies/config_files/javascript_npm.rb' + - 'ee/lib/ai/context/dependencies/config_files/javascript_npm_lock.rb' + - 'ee/lib/ai/context/dependencies/config_files/php_composer.rb' + - 'ee/lib/ai/context/dependencies/config_files/php_composer_lock.rb' + - 'ee/lib/ai/duo_workflow/duo_workflow_service/client.rb' + - 'ee/lib/analytics/ai_user_metrics_database_write_buffer.rb' + - 'ee/lib/analytics/database_write_buffer.rb' + - 'ee/lib/api/ai/duo_workflows/workflows_internal.rb' + - 'ee/lib/audit_events/strategies/base_amazon_s3_destination_strategy.rb' + - 'ee/lib/audit_events/strategies/base_google_cloud_logging_destination_strategy.rb' + - 'ee/lib/audit_events/streaming/destinations/amazon_s3_stream_destination.rb' + - 'ee/lib/audit_events/streaming/destinations/google_cloud_logging_stream_destination.rb' + - 'ee/lib/ee/gitlab/background_migration/fix_string_config_hashes_group_streaming_destinations.rb' + - 'ee/lib/ee/gitlab/background_migration/fix_string_config_hashes_instance_streaming_destinations.rb' + - 'ee/lib/gitlab/ci/parsers/license_compliance/license_scanning.rb' + - 'ee/lib/gitlab/ci/parsers/requirements_management/requirement.rb' + - 'ee/lib/gitlab/ci/parsers/sbom/cyclonedx.rb' + - 'ee/lib/gitlab/duo/administration/verify_self_hosted_setup.rb' + - 'ee/lib/gitlab/duo/chat/agent_event_parser.rb' + - 'ee/lib/gitlab/duo/chat/dataset_reader.rb' + - 'ee/lib/gitlab/geo/signed_data.rb' + - 'ee/lib/gitlab/git_guardian/client.rb' + - 'ee/lib/gitlab/graphql/pagination/elastic_connection.rb' + - 'ee/lib/gitlab/llm/ai_gateway/code_suggestions_client.rb' + - 'ee/lib/gitlab/llm/ai_gateway/completions/categorize_question.rb' + - 'ee/lib/gitlab/llm/ai_gateway/response_modifiers/resolve_vulnerability.rb' + - 'ee/lib/gitlab/llm/anthropic/client.rb' + - 'ee/lib/gitlab/llm/base_response_modifier.rb' + - 'ee/lib/gitlab/llm/chain/tools/identifier.rb' + - 'ee/lib/gitlab/llm/concerns/ai_gateway_client_concern.rb' + - 'ee/lib/gitlab/package_metadata/connector/ndjson_data_file.rb' + - 'ee/lib/gitlab/search/zoekt/client.rb' + - 'ee/lib/gitlab/spdx/catalogue_gateway.rb' + - 'ee/lib/secrets_management/secrets_manager_client.rb' + - 'ee/lib/security/secret_detection/partner_tokens/base_client.rb' + - 'ee/lib/tasks/gitlab/duo_chat/seed_failed_ci_jobs.rake' + - 'ee/lib/tasks/gitlab/spdx.rake' + - 'ee/spec/components/gitlab_subscriptions/trials/duo_enterprise/trial_form_component_spec.rb' + - 'ee/spec/components/gitlab_subscriptions/trials/duo_pro/trial_form_component_spec.rb' + - 'ee/spec/components/gitlab_subscriptions/trials/welcome/trial_form_component_spec.rb' + - 'ee/spec/controllers/ee/projects/pipelines_controller_spec.rb' + - 'ee/spec/controllers/groups/security/policies_controller_spec.rb' + - 'ee/spec/controllers/user_settings/active_sessions_controller_spec.rb' + - 'ee/spec/factories/remote_development/workspace_agentk_states.rb' + - 'ee/spec/factories/spdx_catalogue.rb' + - 'ee/spec/factories/vulnerabilities/findings.rb' + - 'ee/spec/features/billings/qrtly_reconciliation_alert_spec.rb' + - 'ee/spec/features/merge_request/user_sees_security_widget_spec.rb' + - 'ee/spec/features/projects/product_analytics/dashboards_shared_examples.rb' + - 'ee/spec/features/remote_development/workspaces_spec.rb' + - 'ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb' + - 'ee/spec/finders/status_page/incident_comments_finder_spec.rb' + - 'ee/spec/graphql/api/vulnerabilities_spec.rb' + - 'ee/spec/graphql/mutations/security/finding/create_merge_request_spec.rb' + - 'ee/spec/graphql/types/json_string_type_spec.rb' + - 'ee/spec/helpers/ee/integrations_helper_spec.rb' + - 'ee/spec/helpers/ee/invite_members_helper_spec.rb' + - 'ee/spec/helpers/ee/operations_helper_spec.rb' + - 'ee/spec/helpers/ee/organizations/organization_helper_spec.rb' + - 'ee/spec/helpers/ee/projects/pipeline_helper_spec.rb' + - 'ee/spec/helpers/ee/projects/project_members_helper_spec.rb' + - 'ee/spec/helpers/groups/security_features_helper_spec.rb' + - 'ee/spec/helpers/projects/learn_gitlab_helper_spec.rb' + - 'ee/spec/helpers/projects_helper_spec.rb' + - 'ee/spec/helpers/search_helper_spec.rb' + - 'ee/spec/helpers/security_helper_spec.rb' + - 'ee/spec/helpers/users/identity_verification_helper_spec.rb' + - 'ee/spec/helpers/virtual_registry_helper_spec.rb' + - 'ee/spec/helpers/vulnerabilities_helper_spec.rb' + - 'ee/spec/lib/ai/context/dependencies/config_files/base_spec.rb' + - 'ee/spec/lib/api/helpers/audit_events_cursor_helper_spec.rb' + - 'ee/spec/lib/arkose/data_exchange_payload_spec.rb' + - 'ee/spec/lib/arkose/logger_spec.rb' + - 'ee/spec/lib/arkose/verify_response_spec.rb' + - 'ee/spec/lib/audit_events/streaming/destinations/base_stream_destination_spec.rb' + - 'ee/spec/lib/audit_events/streaming/destinations/google_cloud_logging_stream_destination_spec.rb' + - 'ee/spec/lib/ee/api/helpers_spec.rb' + - 'ee/spec/lib/ee/gitlab/background_migration/backfill_workspace_agentk_states_spec.rb' + - 'ee/spec/lib/ee/gitlab/ci/parsers/security/common_spec.rb' + - 'ee/spec/lib/ee/gitlab/ci/pipeline/chain/validate/external_spec.rb' + - 'ee/spec/lib/ee/gitlab/import_export/group/tree_saver_spec.rb' + - 'ee/spec/lib/gitlab/alert_management/alert_payload_field_extractor_spec.rb' + - 'ee/spec/lib/gitlab/ci/google_cloud/generate_build_environment_variables_service_spec.rb' + - 'ee/spec/lib/gitlab/ci/parsers/security/container_scanning_spec.rb' + - 'ee/spec/lib/gitlab/ci/parsers/security/dependency_scanning_spec.rb' + - 'ee/spec/lib/gitlab/cube_js/data_transformer_spec.rb' + - 'ee/spec/lib/gitlab/elastic/bulk_indexer_spec.rb' + - 'ee/spec/lib/gitlab/geo/oauth/session_spec.rb' + - 'ee/spec/lib/gitlab/geo/signed_data_spec.rb' + - 'ee/spec/lib/gitlab/geo_spec.rb' + - 'ee/spec/lib/gitlab/llm/ai_gateway/agent_platform/model_metadata_spec.rb' + - 'ee/spec/lib/gitlab/llm/ai_gateway/client_spec.rb' + - 'ee/spec/lib/gitlab/llm/concerns/exponential_backoff_spec.rb' + - 'ee/spec/lib/gitlab/llm/vertex_ai/client_spec.rb' + - 'ee/spec/lib/gitlab/search/zoekt/response_spec.rb' + - 'ee/spec/lib/gitlab/spdx/catalogue_gateway_spec.rb' + - 'ee/spec/lib/gitlab/spdx/catalogue_spec.rb' + - 'ee/spec/lib/sbom/spdx_spec.rb' + - 'ee/spec/lib/search/elastic/filters/confidentiality_filters_spec.rb' + - 'ee/spec/lib/search/elastic/filters_spec.rb' + - 'ee/spec/lib/search/zoekt/code_query_builder_spec.rb' + - 'ee/spec/lib/search/zoekt/multi_match_spec.rb' + - 'ee/spec/models/ci/build_spec.rb' + - 'ee/spec/models/ee/user_detail_spec.rb' + - 'ee/spec/models/remote_development/workspace_agentk_state_spec.rb' + - 'ee/spec/models/security/orchestration_policy_configuration_spec.rb' + - 'ee/spec/models/users/arkose_sessions_spec.rb' + - 'ee/spec/presenters/audit_event_presenter_spec.rb' + - 'ee/spec/presenters/ee/projects/security/configuration_presenter_spec.rb' + - 'ee/spec/presenters/epic_presenter_spec.rb' + - 'ee/spec/presenters/onboarding/get_started_presenter_spec.rb' + - 'ee/spec/presenters/vulnerabilities/finding_presenter_spec.rb' + - 'ee/spec/requests/admin/ai/amazon_q_settings_controller_spec.rb' + - 'ee/spec/requests/admin/ai/duo_workflow_settings_controller_spec.rb' + - 'ee/spec/requests/api/ai/duo_workflows/workflows_internal_spec.rb' + - 'ee/spec/requests/api/ai/duo_workflows/workflows_spec.rb' + - 'ee/spec/requests/api/code_suggestions_spec.rb' + - 'ee/spec/requests/api/dependency_list_exports_spec.rb' + - 'ee/spec/requests/api/duo_code_review_spec.rb' + - 'ee/spec/requests/api/epics_spec.rb' + - 'ee/spec/requests/api/geo_spec.rb' + - 'ee/spec/requests/api/graphql/boards/board_lists_query_spec.rb' + - 'ee/spec/requests/api/graphql/compliance_management/compliance_requirements_spec.rb' + - 'ee/spec/requests/api/graphql/container_repository/container_repository_details_spec.rb' + - 'ee/spec/requests/api/graphql/group/value_stream_dashboard_usage_overview_spec.rb' + - 'ee/spec/requests/api/graphql/mutations/alert_management/http_integration/create_spec.rb' + - 'ee/spec/requests/api/graphql/mutations/alert_management/http_integration/update_spec.rb' + - 'ee/spec/requests/api/graphql/project/value_stream_dashboard_usage_overview_spec.rb' + - 'ee/spec/requests/api/graphql/vulnerabilities/details_spec.rb' + - 'ee/spec/requests/api/groups_spec.rb' + - 'ee/spec/requests/api/mcp_context_exclusion_spec.rb' + - 'ee/spec/requests/api/projects_spec.rb' + - 'ee/spec/requests/api/search_spec.rb' + - 'ee/spec/requests/api/virtual_registries/cleanup/policies_spec.rb' + - 'ee/spec/requests/api/virtual_registries/container/cache/entries_spec.rb' + - 'ee/spec/requests/api/virtual_registries/container/registries_spec.rb' + - 'ee/spec/requests/api/virtual_registries/container/registry_upstreams_spec.rb' + - 'ee/spec/requests/api/virtual_registries/container/upstreams_spec.rb' + - 'ee/spec/requests/api/virtual_registries/packages/maven/cache/entries_spec.rb' + - 'ee/spec/requests/api/virtual_registries/packages/maven/registries_spec.rb' + - 'ee/spec/requests/api/virtual_registries/packages/maven/registry_upstreams_spec.rb' + - 'ee/spec/requests/api/virtual_registries/packages/maven/upstreams_spec.rb' + - 'ee/spec/requests/custom_roles/regular/admin_cicd_variables/groups_request_spec.rb' + - 'ee/spec/requests/custom_roles/regular/admin_cicd_variables/projects_request_spec.rb' + - 'ee/spec/requests/custom_roles/regular/manage_project_access_tokens/request_spec.rb' + - 'ee/spec/requests/groups/dependencies_controller_spec.rb' + - 'ee/spec/requests/groups/security/compliance_dashboard/frameworks_controller_spec.rb' + - 'ee/spec/requests/projects/security/dast_site_profiles_controller_spec.rb' + - 'ee/spec/requests/projects/security/policies_controller_spec.rb' + - 'ee/spec/serializers/sbom/sbom_entity_spec.rb' + - 'ee/spec/services/ai/active_context/code/deleter_spec.rb' + - 'ee/spec/services/ai/active_context/code/indexer_spec.rb' + - 'ee/spec/services/ai/duo_workflows/agentic_chat_model_metadata_service_spec.rb' + - 'ee/spec/services/ai/duo_workflows/start_workflow_service_spec.rb' + - 'ee/spec/services/alert_management/extract_alert_payload_fields_service_spec.rb' + - 'ee/spec/services/arkose/record_user_data_service_spec.rb' + - 'ee/spec/services/arkose/token_verification_service_spec.rb' + - 'ee/spec/services/compliance_management/compliance_framework/compliance_requirements/trigger_external_control_service_spec.rb' + - 'ee/spec/services/compliance_management/frameworks/json_export_service_spec.rb' + - 'ee/spec/services/compliance_management/frameworks/json_import_service_spec.rb' + - 'ee/spec/services/dependencies/export_serializers/sbom/pipeline_service_spec.rb' + - 'ee/spec/services/geo/container_repository_sync_spec.rb' + - 'ee/spec/services/projects/update_mirror_service_spec.rb' + - 'ee/spec/services/sbom/exporters/cyclonedx/v16_json_service_spec.rb' + - 'ee/spec/services/sbom/exporters/dependency_list_service_spec.rb' + - 'ee/spec/services/sbom/exporters/json_array_service_spec.rb' + - 'ee/spec/services/security/purge_scans_service_spec.rb' + - 'ee/spec/services/security/secret_detection/update_token_status_service_spec.rb' + - 'ee/spec/services/vulnerabilities/apply_llm_remediation_service_spec.rb' + - 'ee/spec/support/helpers/duo_chat_fixture_helpers.rb' + - 'ee/spec/support/helpers/elasticsearch_helpers.rb' + - 'ee/spec/support/helpers/subscription_portal_helpers.rb' + - 'ee/spec/support/shared_contexts/lib/sbom/package_url_shared_contexts.rb' + - 'ee/spec/support/shared_contexts/remote_development/remote_development_shared_contexts.rb' + - 'ee/spec/support/shared_examples/components/ultimate_trial_form_shared_examples.rb' + - 'ee/spec/support/shared_examples/components/welcome_trial_form_shared_examples.rb' + - 'ee/spec/support/shared_examples/lib/audit_events/strategies/google_cloud_logging_destination_strategy_shared_examples.rb' + - 'ee/spec/support/shared_examples/lib/code_suggestions/task_shared_examples.rb' + - 'ee/spec/support/shared_examples/requests/api/graphql/geo/registries_shared_examples.rb' + - 'ee/spec/support/shared_examples/requests/controllers/contribution_analytics_charts_configuration_shared_examples.rb' + - 'ee/spec/support/shared_examples/services/secrets_management/secrets_permissions_update_service_examples.rb' + - 'ee/spec/views/profiles/billings/index.html.haml_spec.rb' + - 'ee/spec/workers/analytics/value_stream_dashboard/count_worker_spec.rb' + - 'ee/spec/workers/click_house/rebuild_materialized_view_cron_worker_spec.rb' + - 'ee/spec/workers/product_analytics/move_funnels_worker_spec.rb' + - 'ee/spec/workers/vulnerabilities/archival/schedule_worker_spec.rb' + - 'keeps/delete_old_feature_flags.rb' + - 'keeps/helpers/ai_editor.rb' + - 'lib/api/helpers/internal_helpers.rb' + - 'lib/banzai/filter/json_table_filter.rb' + - 'lib/bulk_imports/common/pipelines/lfs_objects_pipeline.rb' + - 'lib/ci/inputs/base_input.rb' + - 'lib/ci/retention_policies/pipeline_deletion_cutoff_cache.rb' + - 'lib/click_house/write_buffer.rb' + - 'lib/container_registry/config.rb' + - 'lib/gitlab/auth/otp/strategies/duo_auth/manual_otp.rb' + - 'lib/gitlab/auth/otp/strategies/forti_token_cloud.rb' + - 'lib/gitlab/background_migration/migrate_evidences_for_vulnerability_findings.rb' + - 'lib/gitlab/background_migration/migrate_remediations_for_vulnerability_findings.rb' + - 'lib/gitlab/beyond_identity/client.rb' + - 'lib/gitlab/cache/json_caches/json_keyed.rb' + - 'lib/gitlab/cache/json_caches/redis_keyed.rb' + - 'lib/gitlab/chat_name_token.rb' + - 'lib/gitlab/ci/ansi2html.rb' + - 'lib/gitlab/ci/ansi2json/state.rb' + - 'lib/gitlab/ci/build/artifacts/metadata.rb' + - 'lib/gitlab/ci/components/usages/aggregators/cursor.rb' + - 'lib/gitlab/ci/parsers/accessibility/pa11y.rb' + - 'lib/gitlab/ci/parsers/codequality/code_climate.rb' + - 'lib/gitlab/ci/parsers/terraform/tfplan.rb' + - 'lib/gitlab/consul/internal.rb' + - 'lib/gitlab/database/stat_activity.rb' + - 'lib/gitlab/dependency_linker/json_linker.rb' + - 'lib/gitlab/diff/highlight_cache.rb' + - 'lib/gitlab/discussions_diff/highlight_cache.rb' + - 'lib/gitlab/error_tracking/context_payload_generator.rb' + - 'lib/gitlab/error_tracking/error_repository/open_api_strategy.rb' + - 'lib/gitlab/event_store/event.rb' + - 'lib/gitlab/external_authorization/response.rb' + - 'lib/gitlab/github_import/events_cache.rb' + - 'lib/gitlab/graphql/pagination/click_house_connection.rb' + - 'lib/gitlab/graphql/pagination/keyset/connection.rb' + - 'lib/gitlab/health_checks/puma_check.rb' + - 'lib/gitlab/import/user_from_mention.rb' + - 'lib/gitlab/import_export/json/ndjson_reader.rb' + - 'lib/gitlab/import_export/lfs_restorer.rb' + - 'lib/gitlab/kas/user_access.rb' + - 'lib/gitlab/lfs/client.rb' + - 'lib/gitlab/manifest_import/metadata.rb' + - 'lib/gitlab/merge_requests/mergeability/redis_interface.rb' + - 'lib/gitlab/metrics/samplers/puma_sampler.rb' + - 'lib/gitlab/pagination/keyset/paginator.rb' + - 'lib/gitlab/performance_bar/stats.rb' + - 'lib/gitlab/prometheus_client.rb' + - 'lib/gitlab/redis/cursor_store.rb' + - 'lib/gitlab/sanitizers/exif.rb' + - 'lib/gitlab/sidekiq_daemon/monitor.rb' + - 'lib/gitlab/sidekiq_middleware/concurrency_limit/queue_manager.rb' + - 'lib/gitlab/sidekiq_middleware/concurrency_limit/worker_execution_tracker.rb' + - 'lib/gitlab/sidekiq_middleware/pause_control/pause_control_service.rb' + - 'lib/gitlab/sidekiq_middleware/size_limiter/compressor.rb' + - 'lib/gitlab/sidekiq_migrate_jobs.rb' + - 'lib/gitlab/sidekiq_sharding/router.rb' + - 'lib/gitlab/tracking/destinations/snowplow_context_validator.rb' + - 'lib/gitlab/usage/metric_definition.rb' + - 'lib/gitlab/webpack/manifest.rb' + - 'lib/kramdown/parser/atlassian_document_format.rb' + - 'lib/observability/o11y_token.rb' + - 'lib/tasks/ci/validate_id_token_configuration_task.rb' + - 'lib/tasks/gitlab/background_migrations.rake' + - 'lib/tasks/gitlab/openapi.rake' + - 'spec/components/diffs/stats_component_spec.rb' + - 'spec/components/rapid_diffs/app_component_spec.rb' + - 'spec/components/rapid_diffs/diff_file_header_component_spec.rb' + - 'spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb' + - 'spec/components/rapid_diffs/shared.rb' + - 'spec/components/rapid_diffs/viewers/image_view_component_spec.rb' + - 'spec/controllers/chaos_controller_spec.rb' + - 'spec/controllers/concerns/homepage_data_spec.rb' + - 'spec/controllers/concerns/spammable_actions/captcha_check/rest_api_actions_support_spec.rb' + - 'spec/controllers/import/manifest_controller_spec.rb' + - 'spec/controllers/metrics_controller_spec.rb' + - 'spec/controllers/oauth/token_info_controller_spec.rb' + - 'spec/controllers/projects/artifacts_controller_spec.rb' + - 'spec/controllers/projects/ci/lints_controller_spec.rb' + - 'spec/controllers/projects/cycle_analytics/events_controller_spec.rb' + - 'spec/factories/error_tracking/error_event.rb' + - 'spec/factories/packages/conan/packages.rb' + - 'spec/features/breadcrumbs_schema_markup_spec.rb' + - 'spec/features/error_tracking/user_filters_errors_by_status_spec.rb' + - 'spec/features/error_tracking/user_sees_error_index_spec.rb' + - 'spec/features/frequently_visited_projects_and_groups_spec.rb' + - 'spec/features/projects/import_export/export_file_spec.rb' + - 'spec/features/projects/settings/monitor_settings_spec.rb' + - 'spec/frontend/fixtures/search_navigation.rb' + - 'spec/helpers/admin/abuse_reports_helper_spec.rb' + - 'spec/helpers/admin/broadcast_messages_helper_spec.rb' + - 'spec/helpers/admin/groups_helper_spec.rb' + - 'spec/helpers/admin/projects_helper_spec.rb' + - 'spec/helpers/admin/users_helper_spec.rb' + - 'spec/helpers/breadcrumbs_helper_spec.rb' + - 'spec/helpers/groups_helper_spec.rb' + - 'spec/helpers/ide_helper_spec.rb' + - 'spec/helpers/jira_connect_helper_spec.rb' + - 'spec/helpers/organizations/organization_helper_spec.rb' + - 'spec/helpers/profiles_helper_spec.rb' + - 'spec/helpers/projects/ml/experiments_helper_spec.rb' + - 'spec/helpers/projects/ml/model_registry_helper_spec.rb' + - 'spec/helpers/projects/project_members_helper_spec.rb' + - 'spec/helpers/projects_helper_spec.rb' + - 'spec/helpers/releases_helper_spec.rb' + - 'spec/helpers/search_helper_spec.rb' + - 'spec/helpers/sessions_helper_spec.rb' + - 'spec/helpers/terms_helper_spec.rb' + - 'spec/helpers/tree_helper_spec.rb' + - 'spec/helpers/users_helper_spec.rb' + - 'spec/helpers/workhorse_helper_spec.rb' + - 'spec/lib/api/entities/merge_request_basic_spec.rb' + - 'spec/lib/api/helpers/common_helpers_spec.rb' + - 'spec/lib/api/helpers/rate_limiter_integration_spec.rb' + - 'spec/lib/api/helpers_spec.rb' + - 'spec/lib/atlassian/jira_connect/serializers/deployment_entity_spec.rb' + - 'spec/lib/atlassian/jira_connect/serializers/feature_flag_entity_spec.rb' + - 'spec/lib/atlassian/jira_connect/serializers/repository_entity_spec.rb' + - 'spec/lib/bitbucket/exponential_backoff_spec.rb' + - 'spec/lib/bitbucket_server/representation/activity_spec.rb' + - 'spec/lib/bitbucket_server/representation/comment_spec.rb' + - 'spec/lib/bitbucket_server/representation/pull_request_comment_spec.rb' + - 'spec/lib/bitbucket_server/representation/pull_request_spec.rb' + - 'spec/lib/bitbucket_server/representation/repo_spec.rb' + - 'spec/lib/bulk_imports/logger_spec.rb' + - 'spec/lib/bulk_imports/projects/pipelines/snippets_pipeline_spec.rb' + - 'spec/lib/ci/job_token/policies_spec.rb' + - 'spec/lib/container_registry/gitlab_api_client_spec.rb' + - 'spec/lib/error_tracking/sentry_client/event_spec.rb' + - 'spec/lib/error_tracking/sentry_client/issue_link_spec.rb' + - 'spec/lib/error_tracking/sentry_client/issue_spec.rb' + - 'spec/lib/error_tracking/sentry_client/projects_spec.rb' + - 'spec/lib/error_tracking/sentry_client/repo_spec.rb' + - 'spec/lib/error_tracking/stacktrace_builder_spec.rb' + - 'spec/lib/gitlab/app_json_logger_spec.rb' + - 'spec/lib/gitlab/bitbucket_server_import/importers/pull_request_importer_spec.rb' + - 'spec/lib/gitlab/ci/parsers/security/common_spec.rb' + - 'spec/lib/gitlab/ci/pipeline/chain/validate/external_spec.rb' + - 'spec/lib/gitlab/database/load_balancing/srv_resolver_spec.rb' + - 'spec/lib/gitlab/database/migrations/instrumentation_spec.rb' + - 'spec/lib/gitlab/database/migrations/observers/batch_details_spec.rb' + - 'spec/lib/gitlab/database/migrations/observers/query_details_spec.rb' + - 'spec/lib/gitlab/database/migrations/observers/transaction_duration_spec.rb' + - 'spec/lib/gitlab/database/migrations/runner_spec.rb' + - 'spec/lib/gitlab/database/migrations/test_batched_background_runner_spec.rb' + - 'spec/lib/gitlab/database/stat_activity_spec.rb' + - 'spec/lib/gitlab/diff/position_spec.rb' + - 'spec/lib/gitlab/error_tracking/error_repository/open_api_strategy_spec.rb' + - 'spec/lib/gitlab/grape_logging/formatters/lograge_with_timestamp_spec.rb' + - 'spec/lib/gitlab/graphql/pagination/click_house_aggregated_connection_spec.rb' + - 'spec/lib/gitlab/graphql/pagination/click_house_connection_spec.rb' + - 'spec/lib/gitlab/graphql/pagination/keyset/connection_spec.rb' + - 'spec/lib/gitlab/graphql/subscriptions/action_cable_with_load_balancing_spec.rb' + - 'spec/lib/gitlab/graphql_logger_spec.rb' + - 'spec/lib/gitlab/import_export/group/tree_restorer_spec.rb' + - 'spec/lib/gitlab/import_export/import_test_coverage_spec.rb' + - 'spec/lib/gitlab/import_export/json/ndjson_reader_spec.rb' + - 'spec/lib/gitlab/import_export/lfs_saver_spec.rb' + - 'spec/lib/gitlab/import_export/project/relation_saver_spec.rb' + - 'spec/lib/gitlab/legacy_github_import/importer_spec.rb' + - 'spec/lib/gitlab/middleware/json_validation_spec.rb' + - 'spec/lib/gitlab/prometheus_client_spec.rb' + - 'spec/lib/gitlab/reflections/relationships/relationship_spec.rb' + - 'spec/lib/gitlab/sidekiq_logging/json_formatter_spec.rb' + - 'spec/lib/gitlab/sidekiq_migrate_jobs_spec.rb' + - 'spec/lib/gitlab/webpack/manifest_spec.rb' + - 'spec/lib/gitlab/workhorse_spec.rb' + - 'spec/lib/pager_duty/webhook_payload_parser_spec.rb' + - 'spec/lib/serializers/unsafe_json_spec.rb' + - 'spec/lib/service_ping/devops_report_spec.rb' + - 'spec/models/ci/job_token/authorization_spec.rb' + - 'spec/models/container_repository_spec.rb' + - 'spec/models/packages/composer/sti/package_spec.rb' + - 'spec/models/packages/dependency_link_spec.rb' + - 'spec/models/snippet_spec.rb' + - 'spec/models/supply_chain/slsa/provenance_statement_spec.rb' + - 'spec/presenters/ml/candidate_details_presenter_spec.rb' + - 'spec/presenters/projects/security/configuration_presenter_spec.rb' + - 'spec/requests/admin/broadcast_messages_controller_spec.rb' + - 'spec/requests/api/ci/jobs_spec.rb' + - 'spec/requests/api/ci/runner_job_confirmation_integration_spec.rb' + - 'spec/requests/api/graphql/container_repository/container_repository_details_spec.rb' + - 'spec/requests/api/graphql/mutations/issues/link_alerts_spec.rb' + - 'spec/requests/api/graphql/mutations/issues/unlink_alerts_spec.rb' + - 'spec/requests/api/graphql/project/alert_management/alert/assignees_spec.rb' + - 'spec/requests/api/graphql/project/alert_management/alert/issue_spec.rb' + - 'spec/requests/api/graphql/project/alert_management/alert/notes_spec.rb' + - 'spec/requests/api/graphql/project/alert_management/alert/todos_spec.rb' + - 'spec/requests/api/graphql/project/issue/design_collection/version_spec.rb' + - 'spec/requests/api/graphql/project/issue/design_collection/versions_spec.rb' + - 'spec/requests/api/graphql/project/issue/designs/designs_spec.rb' + - 'spec/requests/api/groups_spec.rb' + - 'spec/requests/api/internal/mail_room_spec.rb' + - 'spec/requests/api/mcp/base_spec.rb' + - 'spec/requests/api/ml/mlflow/experiments_spec.rb' + - 'spec/requests/api/ml/mlflow/model_versions_spec.rb' + - 'spec/requests/api/ml/mlflow/registered_models_spec.rb' + - 'spec/requests/api/ml/mlflow/runs_spec.rb' + - 'spec/requests/api/ml/mlflow_artifacts/artifacts_spec.rb' + - 'spec/requests/api/npm_project_packages_spec.rb' + - 'spec/requests/api/project_container_repositories_spec.rb' + - 'spec/requests/api/projects_spec.rb' + - 'spec/requests/api/repositories_spec.rb' + - 'spec/requests/api/search_spec.rb' + - 'spec/requests/api/terraform/state_spec.rb' + - 'spec/requests/api/usage_data_queries_spec.rb' + - 'spec/requests/git_http_spec.rb' + - 'spec/requests/jwks_controller_spec.rb' + - 'spec/requests/organizations/organizations_controller_spec.rb' + - 'spec/requests/projects/commit_controller_spec.rb' + - 'spec/requests/projects/incident_management/pagerduty_incidents_spec.rb' + - 'spec/requests/projects/issues_controller_spec.rb' + - 'spec/requests/projects/merge_requests_controller_spec.rb' + - 'spec/requests/projects/packages/package_files_controller_spec.rb' + - 'spec/requests/projects/service_desk_controller_spec.rb' + - 'spec/requests/pwa_controller_spec.rb' + - 'spec/requests/users_controller_spec.rb' + - 'spec/scripts/internal_events/server_spec.rb' + - 'spec/serializers/ci/daily_build_group_report_result_serializer_spec.rb' + - 'spec/services/authn/passkey/register_service_spec.rb' + - 'spec/services/bulk_imports/lfs_objects_export_service_spec.rb' + - 'spec/services/ci/pipeline_triggers/create_service_spec.rb' + - 'spec/services/ci/register_job_service_spec.rb' + - 'spec/services/ci/register_job_service_two_phase_commit_spec.rb' + - 'spec/services/incident_management/pager_duty/create_incident_issue_service_spec.rb' + - 'spec/services/incident_management/pager_duty/process_webhook_service_spec.rb' + - 'spec/services/packages/conan/metadata_extraction_service_spec.rb' + - 'spec/services/packages/create_dependency_service_spec.rb' + - 'spec/services/packages/npm/create_metadata_cache_service_spec.rb' + - 'spec/services/packages/npm/create_package_service_spec.rb' + - 'spec/services/packages/terraform_module/metadata/update_service_spec.rb' + - 'spec/services/webauthn/register_service_spec.rb' + - 'spec/support/database/auto_explain.rb' + - 'spec/support/helpers/ci/template_helpers.rb' + - 'spec/support/helpers/graphql_helpers.rb' + - 'spec/support/helpers/stub_gitlab_calls.rb' + - 'spec/support/helpers/workhorse_helpers.rb' + - 'spec/support/import_export/common_util.rb' + - 'spec/support/import_export/configuration_helper.rb' + - 'spec/support/matchers/be_valid_json.rb' + - 'spec/support/matchers/disallow_request_matchers.rb' + - 'spec/support/matchers/schema_matcher.rb' + - 'spec/support/shared_contexts/container_registry_tags_shared_context.rb' + - 'spec/support/shared_contexts/controllers/logging_shared_context.rb' + - 'spec/support/shared_contexts/features/error_tracking_shared_context.rb' + - 'spec/support/shared_contexts/json_response_shared_context.rb' + - 'spec/support/shared_contexts/requests/api/oidc_discovery_shared_context.rb' + - 'spec/support/shared_contexts/services/packages/rpm/xml_shared_context.rb' + - 'spec/support/shared_examples/features/access_tokens_shared_examples.rb' + - 'spec/support/shared_examples/graphql/mutations/http_integrations_shared_examples.rb' + - 'spec/support/shared_examples/harbor/artifacts_controller_shared_examples.rb' + - 'spec/support/shared_examples/integrations/integration_settings_form.rb' + - 'spec/support/shared_examples/lib/cache_helpers_shared_examples.rb' + - 'spec/support/shared_examples/lib/gitlab/json_logger_shared_examples.rb' + - 'spec/support/shared_examples/models/chat_integration_shared_examples.rb' + - 'spec/support/shared_examples/models/concerns/integrations/base/discord_shared_examples.rb' + - 'spec/support/shared_examples/models/concerns/integrations/base/hangouts_chat_shared_examples.rb' + - 'spec/support/shared_examples/models/concerns/integrations/base/irker_shared_examples.rb' + - 'spec/support/shared_examples/requests/access_tokens_controller_shared_examples.rb' + - 'spec/support/shared_examples/requests/api/container_repositories_shared_examples.rb' + - 'spec/support/shared_examples/requests/api/issuable_update_shared_examples.rb' + - 'spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb' + - 'spec/support/shared_examples/requests/api_keyset_pagination_shared_examples.rb' + - 'spec/tasks/database_relationships_spec.rb' + - 'spec/workers/database/collation_checker_worker_spec.rb' + - 'spec/workers/database/schema_checker_worker_spec.rb' diff --git a/doc/development/secure_coding_guidelines/_index.md b/doc/development/secure_coding_guidelines/_index.md index 4d48e4cdf4eb02..ef17d1d12be0de 100644 --- a/doc/development/secure_coding_guidelines/_index.md +++ b/doc/development/secure_coding_guidelines/_index.md @@ -204,6 +204,91 @@ When working with regular expressions in Python, use `re2` when possible or alwa - [The impact of regular expression denial of service (ReDoS) in practice: an empirical study at the ecosystem scale](https://davisjam.github.io/files/publications/DavisCoghlanServantLee-EcosystemREDOS-ESECFSE18.pdf). This research paper discusses approaches to automatically detect ReDoS vulnerabilities. - [Freezing the web: A study of ReDoS vulnerabilities in JavaScript-based web servers](https://www.usenix.org/system/files/conference/usenixsecurity18/sec18-staicu.pdf). Another research paper about detecting ReDoS vulnerabilities. +## JSON Parsing + +### Description + +Parsing untrusted JSON input without size or depth limits can lead to denial of service (DoS) vulnerabilities. Malicious payloads with deeply nested structures, extremely large arrays, or oversized documents can exhaust server memory or CPU resources. + +### Impact + +- **Memory exhaustion**: Large arrays or hashes can consume excessive memory, potentially crashing the application. +- **Stack exhaustion**: Deeply nested structures can cause stack overflow errors. +- **CPU exhaustion**: Processing extremely large JSON documents ties up server resources. +- **Denial of service**: Attackers can exploit these weaknesses to make the application unavailable. + +### When to consider + +When parsing JSON from any untrusted source, including: + +- HTTP request bodies +- User-supplied parameters +- Webhook payloads +- External API responses +- User-uploaded files +- Data from message queues + +### Mitigation + +Use `Gitlab::Json.safe_parse` instead of `Gitlab::Json.parse` when handling untrusted input. The `safe_parse` method enforces limits on: + +| Limit | Default | Description | +|-------|---------|-------------| +| `max_depth` | 32 | Maximum nesting depth | +| `max_array_size` | 50,000 | Maximum elements per array | +| `max_hash_size` | 50,000 | Maximum key-value pairs per hash | +| `max_total_elements` | 100,000 | Maximum total elements across all arrays and hashes | +| `max_json_size_bytes` | 20 MB | Maximum size of the JSON input | + +#### Examples + +```ruby +# Bad - no protection against malicious payloads +data = Gitlab::Json.parse(request.body.read) + +# Good - enforces default safety limits +data = Gitlab::Json.safe_parse(request.body.read) + +# Good - with custom limits for specific use cases +data = Gitlab::Json.safe_parse( + request.body.read, + parse_limits: { max_depth: 10, max_json_size_bytes: 1.megabyte } +) +``` + +#### Handling parse errors + +`safe_parse` raises `JSON::ParserError` for both malformed JSON and limit violations. The error messages are user-safe and do not expose internal details: + +```ruby +begin + data = Gitlab::Json.safe_parse(user_input) +rescue JSON::ParserError => e + # Error messages are safe to display: + # - "Parameters nested too deeply" + # - "Array parameter too large" + # - "Hash parameter too large" + # - "Too many total parameters" + # - "JSON body too large" + render json: { error: e.message }, status: :bad_request +end +``` + +### When to use `Gitlab::Json.parse` + +Use the standard `parse` method only when you have full control over the input source and trust its contents, such as: + +- Reading from internal configuration files +- Parsing data from trusted internal services with established contracts +- Processing data that has already been validated + +When in doubt, prefer `safe_parse`. + +### Resources + +- [GitLab JSON development guidelines](../json.md) +- [OWASP Input Validation Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Input_Validation_Cheat_Sheet.html) + ## JSON Web Tokens (JWT) ### Description diff --git a/rubocop/cop/gitlab/json_safe_parse.rb b/rubocop/cop/gitlab/json_safe_parse.rb new file mode 100644 index 00000000000000..fa272a4757000f --- /dev/null +++ b/rubocop/cop/gitlab/json_safe_parse.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Gitlab + # Encourages the use of `Gitlab::Json.safe_parse` over `Gitlab::Json.parse` + # for parsing untrusted JSON input with built-in size and depth limits. + # + # `safe_parse` provides protection against: + # - Deeply nested structures (DoS via stack exhaustion) + # - Extremely large arrays or hashes (memory exhaustion) + # - Oversized JSON payloads (memory exhaustion) + # + # @example + # # bad + # Gitlab::Json.parse(user_input) + # Gitlab::Json.parse(request.body.read) + # ::Gitlab::Json.parse(params[:data]) + # + # # good + # Gitlab::Json.safe_parse(user_input) + # Gitlab::Json.safe_parse(request.body.read) + # ::Gitlab::Json.safe_parse(params[:data]) + # + # # also good - with custom limits + # Gitlab::Json.safe_parse(data, parse_limits: { max_depth: 10 }) + # + class JsonSafeParse < RuboCop::Cop::Base + extend RuboCop::Cop::AutoCorrector + + MSG = <<~TEXT.chomp + Prefer `Gitlab::Json.safe_parse` over `Gitlab::Json.parse` when parsing untrusted input. \ + See https://docs.gitlab.com/ee/development/json.html + TEXT + + RESTRICT_ON_SEND = %i[parse parse! load decode].freeze + + # @!method gitlab_json_call(node) + def_node_matcher :gitlab_json_call, <<~PATTERN + (send + (const + (const {nil? (cbase)} :Gitlab) + :Json) + ${:parse :parse! :load :decode} + $...) + PATTERN + + def on_send(node) + method_name, args = gitlab_json_call(node) + return unless method_name + + add_offense(node) do |corrector| + corrector.replace(node, replacement(node, args)) + end + end + alias_method :on_csend, :on_send + + private + + def replacement(node, arg_nodes) + arg_source = arg_nodes.map(&:source).join(", ") + "#{cbase_prefix(node)}Gitlab::Json.safe_parse(#{arg_source})" + end + + def cbase_prefix(node) + return "::" if node.source_range.source_buffer.name.include?('/ee/') + + "" + end + end + end + end +end diff --git a/spec/rubocop/cop/gitlab/json_safe_parse_spec.rb b/spec/rubocop/cop/gitlab/json_safe_parse_spec.rb new file mode 100644 index 00000000000000..a054419665661e --- /dev/null +++ b/spec/rubocop/cop/gitlab/json_safe_parse_spec.rb @@ -0,0 +1,152 @@ +# frozen_string_literal: true + +require 'rubocop_spec_helper' +require_relative '../../../../rubocop/cop/gitlab/json_safe_parse' + +RSpec.describe RuboCop::Cop::Gitlab::JsonSafeParse, feature_category: :tooling do + describe 'autocorrection' do + context 'when using Gitlab::Json.parse' do + it 'corrects to Gitlab::Json.safe_parse' do + expect_offense(<<~RUBY) + Gitlab::Json.parse(user_input) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [...] + RUBY + + expect_correction(<<~RUBY) + Gitlab::Json.safe_parse(user_input) + RUBY + end + end + + context 'when using ::Gitlab::Json.parse' do + it 'corrects to Gitlab::Json.safe_parse' do + expect_offense(<<~RUBY) + ::Gitlab::Json.parse(data) + ^^^^^^^^^^^^^^^^^^^^^^^^^^ [...] + RUBY + + expect_correction(<<~RUBY) + Gitlab::Json.safe_parse(data) + RUBY + end + end + + context 'when using Gitlab::Json.parse!' do + it 'corrects to Gitlab::Json.safe_parse' do + expect_offense(<<~RUBY) + Gitlab::Json.parse!(input) + ^^^^^^^^^^^^^^^^^^^^^^^^^^ [...] + RUBY + + expect_correction(<<~RUBY) + Gitlab::Json.safe_parse(input) + RUBY + end + end + + context 'when using Gitlab::Json.load' do + it 'corrects to Gitlab::Json.safe_parse' do + expect_offense(<<~RUBY) + Gitlab::Json.load(string) + ^^^^^^^^^^^^^^^^^^^^^^^^^ [...] + RUBY + + expect_correction(<<~RUBY) + Gitlab::Json.safe_parse(string) + RUBY + end + end + + context 'when using Gitlab::Json.decode' do + it 'corrects to Gitlab::Json.safe_parse' do + expect_offense(<<~RUBY) + Gitlab::Json.decode(json_string) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [...] + RUBY + + expect_correction(<<~RUBY) + Gitlab::Json.safe_parse(json_string) + RUBY + end + end + + context 'when using parse with multiple arguments' do + it 'preserves all arguments' do + expect_offense(<<~RUBY) + Gitlab::Json.parse(data, symbolize_names: true) + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [...] + RUBY + + expect_correction(<<~RUBY) + Gitlab::Json.safe_parse(data, symbolize_names: true) + RUBY + end + end + + context 'when in an EE file' do + it 'uses :: prefix' do + expect_offense(<<~RUBY, '/path/to/ee/foo.rb') + class Foo + def bar + Gitlab::Json.parse(data) + ^^^^^^^^^^^^^^^^^^^^^^^^ [...] + end + end + RUBY + + expect_correction(<<~RUBY) + class Foo + def bar + ::Gitlab::Json.safe_parse(data) + end + end + RUBY + end + end + end + + describe 'non-offenses' do + it 'does not flag Gitlab::Json.safe_parse' do + expect_no_offenses(<<~RUBY) + Gitlab::Json.safe_parse(user_input) + RUBY + end + + it 'does not flag Gitlab::Json.safe_parse with options' do + expect_no_offenses(<<~RUBY) + Gitlab::Json.safe_parse(data, parse_limits: { max_depth: 10 }) + RUBY + end + + it 'does not flag Gitlab::Json.generate' do + expect_no_offenses(<<~RUBY) + Gitlab::Json.generate(hash) + RUBY + end + + it 'does not flag Gitlab::Json.dump' do + expect_no_offenses(<<~RUBY) + Gitlab::Json.dump(object) + RUBY + end + + it 'does not flag JSON.parse (handled by Gitlab/Json cop)' do + expect_no_offenses(<<~RUBY) + JSON.parse(string) + RUBY + end + + it 'does not flag other parse methods' do + expect_no_offenses(<<~RUBY) + YAML.parse(string) + SomeClass.parse(string) + RUBY + end + + it 'does not flag Gitlab::Json.pretty_generate' do + expect_no_offenses(<<~RUBY) + Gitlab::Json.pretty_generate(hash) + RUBY + end + end +end -- GitLab From 11422ef3e72f6173248d22b46d562fdc575c5f96 Mon Sep 17 00:00:00 2001 From: felix Date: Wed, 3 Dec 2025 13:46:00 -0500 Subject: [PATCH 2/5] change to BLUF format and fix link on cop error --- doc/development/secure_coding_guidelines/_index.md | 4 ++-- rubocop/cop/gitlab/json_safe_parse.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/development/secure_coding_guidelines/_index.md b/doc/development/secure_coding_guidelines/_index.md index ef17d1d12be0de..a1e85240f3ef86 100644 --- a/doc/development/secure_coding_guidelines/_index.md +++ b/doc/development/secure_coding_guidelines/_index.md @@ -206,6 +206,8 @@ When working with regular expressions in Python, use `re2` when possible or alwa ## JSON Parsing +**Use `Gitlab::Json.safe_parse` instead of `Gitlab::Json.parse` when handling untrusted input.** When in doubt, prefer `safe_parse`. + ### Description Parsing untrusted JSON input without size or depth limits can lead to denial of service (DoS) vulnerabilities. Malicious payloads with deeply nested structures, extremely large arrays, or oversized documents can exhaust server memory or CPU resources. @@ -282,8 +284,6 @@ Use the standard `parse` method only when you have full control over the input s - Parsing data from trusted internal services with established contracts - Processing data that has already been validated -When in doubt, prefer `safe_parse`. - ### Resources - [GitLab JSON development guidelines](../json.md) diff --git a/rubocop/cop/gitlab/json_safe_parse.rb b/rubocop/cop/gitlab/json_safe_parse.rb index fa272a4757000f..6425b32fdd92e9 100644 --- a/rubocop/cop/gitlab/json_safe_parse.rb +++ b/rubocop/cop/gitlab/json_safe_parse.rb @@ -30,7 +30,7 @@ class JsonSafeParse < RuboCop::Cop::Base MSG = <<~TEXT.chomp Prefer `Gitlab::Json.safe_parse` over `Gitlab::Json.parse` when parsing untrusted input. \ - See https://docs.gitlab.com/ee/development/json.html + See https://docs.gitlab.com/development/secure_coding_guidelines/#json-parsing TEXT RESTRICT_ON_SEND = %i[parse parse! load decode].freeze -- GitLab From 5f839782134a6ab81bc1442c43152501fef34b29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Veillette-Potvin?= Date: Thu, 11 Dec 2025 08:10:14 -0500 Subject: [PATCH 3/5] Apply 2 suggestion(s) to 2 file(s) Co-authored-by: Thong Kuah --- .rubocop.yml | 1 + doc/development/secure_coding_guidelines/_index.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/.rubocop.yml b/.rubocop.yml index ee9363dbc61958..8c65d934324f45 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -463,6 +463,7 @@ Gitlab/Json: Gitlab/JsonSafeParse: Description: 'Prefer Gitlab::Json.safe_parse over Gitlab::Json.parse for untrusted input' Enabled: true + SafeAutoCorrect: false # Default parse_limits may not work always Exclude: - 'qa/**/*' - 'scripts/**/*' diff --git a/doc/development/secure_coding_guidelines/_index.md b/doc/development/secure_coding_guidelines/_index.md index a1e85240f3ef86..e199488aac87c3 100644 --- a/doc/development/secure_coding_guidelines/_index.md +++ b/doc/development/secure_coding_guidelines/_index.md @@ -260,7 +260,7 @@ data = Gitlab::Json.safe_parse( #### Handling parse errors -`safe_parse` raises `JSON::ParserError` for both malformed JSON and limit violations. The error messages are user-safe and do not expose internal details: +`safe_parse` raises `JSON::ParserError` for both malformed JSON and limit violations. The error messages are user-safe and do not expose internal details. An internal facing error (`Gitlab::Json::StreamValidator::LimitExceededError`) will be logged for inspection. ```ruby begin -- GitLab From a45b00a8ca9c740bc964c0c71bd4a0fbdf5ce30a Mon Sep 17 00:00:00 2001 From: felix Date: Fri, 12 Dec 2025 11:33:57 -0500 Subject: [PATCH 4/5] fix vale error --- doc/development/secure_coding_guidelines/_index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/development/secure_coding_guidelines/_index.md b/doc/development/secure_coding_guidelines/_index.md index e199488aac87c3..4385e67ff049b9 100644 --- a/doc/development/secure_coding_guidelines/_index.md +++ b/doc/development/secure_coding_guidelines/_index.md @@ -260,7 +260,7 @@ data = Gitlab::Json.safe_parse( #### Handling parse errors -`safe_parse` raises `JSON::ParserError` for both malformed JSON and limit violations. The error messages are user-safe and do not expose internal details. An internal facing error (`Gitlab::Json::StreamValidator::LimitExceededError`) will be logged for inspection. +`safe_parse` raises `JSON::ParserError` for both malformed JSON and limit violations. The error messages are user-safe and do not expose internal details. An internal facing error (`Gitlab::Json::StreamValidator::LimitExceededError`) will be logged for inspection. ```ruby begin -- GitLab From ecd93a637a133c4afd399fa412e8f066204b75f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Veillette-Potvin?= Date: Mon, 15 Dec 2025 11:15:02 -0500 Subject: [PATCH 5/5] Apply 1 suggestion(s) to 1 file(s) Co-authored-by: Thong Kuah --- .rubocop.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 8c65d934324f45..b9aaf7082702ba 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -464,11 +464,6 @@ Gitlab/JsonSafeParse: Description: 'Prefer Gitlab::Json.safe_parse over Gitlab::Json.parse for untrusted input' Enabled: true SafeAutoCorrect: false # Default parse_limits may not work always - Exclude: - - 'qa/**/*' - - 'scripts/**/*' - - 'lib/quality/**/*' - - 'tooling/danger/**/*' Gitlab/AvoidUploadedFileFromParams: Enabled: true -- GitLab