Skip to content

CBG-5892: Add config to disable local endpoint for public API - #8841

Merged
bbrks merged 3 commits into
mainfrom
CBG-5892
Oct 2, 2026
Merged

bbrks merged 3 commits into
mainfrom
CBG-5892

Conversation

@RIT3shSapata

@RIT3shSapata RIT3shSapata commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

CBG-5892

Add a startup config option to turn off the _local document endpoints on the public API. The _local endpoints do not enforce write access control, so any authenticated user can store any data in the bucket.

  • New option api.enable_local_endpoint_for_public_api, also available as a CLI flag. If it is not set, the endpoints stay enabled, so the default behavior does not change. The decision about the default is separate from this PR.
  • When the option is false, GET, HEAD, PUT and DELETE on /{keyspace}/_local/{docid} on the public API return 403 with {"error":"Forbidden","reason":"_local endpoint is disabled"}, whatever the Accept header is.
  • When the option is false, a public _bulk_docs request still returns 201 and writes its normal docs. Each _local doc in the request is not written and gets a 403 status in the response.
  • The admin API is not affected.
  • Updated the OpenAPI specs: the new option in Startup-config, the 403 response on the public _local endpoints, and the _bulk_docs behavior.
  • New test TestLocalEndpointForPublicAPI covers the enabled and disabled cases for GET, HEAD, PUT, DELETE and _bulk_docs, a client that does not accept JSON, and the admin API.

Pre-review checklist

  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Dependencies (if applicable)

  • Link upstream PRs
  • Update Go module dependencies when merged

Integration Tests

@RIT3shSapata
RIT3shSapata requested review from a team and a balanced review from Copilot September 30, 2026 12:41
@factory-droid

factory-droid Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Droid finished @RIT3shSapata's task —— View job


Review summary: The overall approach looks sound and closes a real security gap, but the new 403 paths currently depend on Accept: application/json and can return 406 instead. There’s also a serious CLI flag wiring issue in the touched config flags block, and the API specs should be updated to reflect the new conditional behavior.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The OpenAPI startup schema is incomplete, serialization is inconsistent, and HEAD/admin behavior lacks coverage.

Review effort: Balanced
Findings: 1 Medium severity · 2 Low severity

Open (3)
What changed in this PR

Adds a startup option to disable public _local document access while preserving admin API behavior.

Changes:

  • Adds api.enable_local_endpoint_for_public_api.
  • Blocks public _local CRUD and mixed _bulk_docs requests when disabled.
  • Adds enabled/disabled behavior tests.
File Description
rest/​handler.go Detects disabled public _local access.
rest/​doc_api.go Rejects public _local CRUD requests.
rest/​config_startup.go Defines the startup setting and default behavior.
rest/​config_flags.go Registers the corresponding CLI flag.
rest/​bulk_api.go Rejects bulk requests containing local documents.
rest/​api_test.go Tests public endpoint and bulk behavior.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread rest/config_startup.go Outdated
Comment thread rest/api_test.go Outdated
Comment thread rest/config_startup.go Outdated
Comment thread rest/doc_api.go Outdated
Comment thread rest/config_flags.go
Comment thread rest/config_startup.go Outdated
- bulk api now returns row level errors
- fixed the way the endpoints return an error to prevent 406
- added case to test the HEAD request as well
- open  api spec have been updated
- added test case to ensure the admin api is not affected by this change
- added omitempty tag to the config
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

Redocly previews

@RIT3shSapata
RIT3shSapata requested a balanced review from Copilot October 1, 2026 08:08
@RIT3shSapata

Copy link
Copy Markdown
Contributor Author

@droid review

@factory-droid

factory-droid Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Droid finished @RIT3shSapata's task —— View job


Both candidate comments are valid: the new TestLocalEndpointForPublicAPI does not compile due to new(test.enabled), and the public _bulk_docs OpenAPI schema is missing the optional cv field that can be returned on success.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Tests do not fully verify the unset default, CLI false path, required error payload, and prevention of direct mutations.

Review effort: Balanced
Findings: 1 Medium severity · 3 Low severity

Open (4)
Resolved since last review (3)

Comment thread rest/api_test.go
Comment on lines +4491 to +4492
RequireStatus(t, rt.SendUserRequest(http.MethodPut, "/{{.keyspace}}/_local/doc2", `{"key":"value"}`, "alice"), test.expectedPutStatus)
RequireStatus(t, rt.SendUserRequest(http.MethodDelete, "/{{.keyspace}}/_local/doc1", "", "alice"), test.expectedStatus)
Comment thread rest/api_test.go
Comment on lines +4479 to +4480
MutateStartupConfig: func(config *StartupConfig) {
config.API.EnableLocalEndpointForPublicAPI = new(test.enabled)
Comment thread rest/api_test.go

RequireStatus(t, rt.SendUserRequest(http.MethodGet, "/{{.keyspace}}/_local/doc1", "", "alice"), test.expectedStatus)
RequireStatus(t, rt.SendUserRequest(http.MethodHead, "/{{.keyspace}}/_local/doc1", "", "alice"), test.expectedStatus)
RequireStatus(t, rt.SendUserRequestWithHeaders(http.MethodGet, "/{{.keyspace}}/_local/doc1", "", map[string]string{"Accept": "text/html"}, "alice", RestTesterDefaultUserPassword), test.expectedStatus)
Comment thread rest/config_flags.go
"api.admin_interface_authentication": {config: &config.API.AdminInterfaceAuthentication, flagValue: fs.Bool("api.admin_interface_authentication", false, "Whether the admin API requires authentication")},
"api.metrics_interface_authentication": {config: &config.API.MetricsInterfaceAuthentication, flagValue: fs.Bool("api.metrics_interface_authentication", false, "Whether the metrics API requires authentication")},
"api.enable_admin_authentication_permissions_check": {config: &config.API.EnableAdminAuthenticationPermissionsCheck, flagValue: fs.Bool("api.enable_admin_authentication_permissions_check", false, "Whether to enable the DP permissions check feature of admin auth")},
"api.enable_local_endpoint_for_public_api": {config: &config.API.EnableLocalEndpointForPublicAPI, flagValue: fs.Bool("api.enable_local_endpoint_for_public_api", true, "Whether to enable the local endpoint for public API")},
Comment thread rest/api_test.go
Comment thread docs/api/paths/public/keyspace-_bulk_docs.yaml
@bbrks
bbrks merged commit 88e4415 into main Oct 2, 2026
55 checks passed
@bbrks
bbrks deleted the CBG-5892 branch October 2, 2026 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants