Repository navigation
[BREAKING] FIX: Keep a private copy's endpoint and credentials the same as its source's (part 17) - #3984
[BREAKING] FIX: Keep a private copy's endpoint and credentials the same as its source's (part 17)#3984varunj-msft wants to merge 5 commits into
Conversation
A copy built from a registered target or converter through the API could set its own endpoint, auth mode, key or headers, or for the Azure Speech converters its region, key or resource ID, while keeping the source's credential. Those settings are now locked on copies: classes declare them, the registry refuses them for API input with any value, and the converter settings dialog no longer offers them or carries them over from earlier settings. Settings like temperature or the voice can still change.
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues were identified in the changes restricting connection-setting overrides for API-created private copies.
No open issues 🎉
What changed in this PR
This PR prevents API-created private copies of OpenAI targets and Azure Speech converters from overriding connection settings, keeping source credentials tied to their configured destinations. New component creation and trusted Python copies remain unchanged.
Changes:
- Adds
copy_lockedparameter metadata and rejects locked overrides from external input, including unchanged or null values. - Hides locked fields in temporary converter settings and removes legacy overrides before submission.
- Adds regression coverage and documents the breaking API behavior, including the updated no-key copy behavior from #3809.
| File | Description |
|---|---|
tests/unit/registry/test_resolution.py |
Tests class-declared copy-lock metadata. |
tests/unit/registry/test_converter_registry.py |
Tests Speech copy restrictions and permitted overrides. |
tests/unit/models/test_parameter.py |
Checks serialization of copy_locked. |
tests/unit/backend/test_temporary_components.py |
Tests target connection locks, including no-key copies. |
tests/unit/backend/test_technique_edit_service.py |
Tests locked converter overrides in technique edits. |
tests/unit/backend/test_converter_service.py |
Tests restrictions across creation, preview, and editable settings. |
pyrit/registry/resolution.py |
Derives copy-lock metadata from component declarations. |
pyrit/registry/registry.py |
Rejects externally supplied locked overrides. |
pyrit/prompt_target/openai/openai_target.py |
Declares locked OpenAI connection parameters. |
pyrit/prompt_target/common/prompt_target.py |
Defines the target copy-lock declaration. |
pyrit/models/parameter.py |
Adds the serialized copy_locked field. |
pyrit/converter/converter.py |
Defines the converter copy-lock declaration. |
pyrit/converter/azure_speech_text_to_audio_converter.py |
Locks Speech region, key, and resource ID. |
pyrit/converter/azure_speech_audio_to_text_converter.py |
Locks Speech region, key, and resource ID. |
pyrit/backend/services/converter_service.py |
Excludes locked parameters from editable instance settings. |
frontend/src/types/index.ts |
Adds frontend typing for copy_locked. |
frontend/src/components/Registry/CreateConverterDialog.tsx |
Hides locked settings and filters legacy overrides. |
frontend/src/components/Registry/CreateConverterDialog.test.tsx |
Tests copy filtering and unchanged creation controls. |
doc/code/registry/0_registry.md |
Documents connection locks and component declarations. |
Review effort: Balanced
Behnam (behnam-o)
left a comment
There was a problem hiding this comment.
The mechanism is clean: components declare the settings a copy can't change, the registry enforces them for API input, and the UI only hides them. I traced every API path that builds a copy: target create from a source, converter create and preview, and technique edits. All of them enforce the lock, and the current lists cover every copyable class that holds credentials.
I don't think the risk in the description is reachable today, though:
- Target copies with these overrides never send. A copy must use
register=false, so the backend returns its identifier and capabilities and releases it. The only target copies that send are saved bindings that carry onlytemperature. Also, a new target can already send the server's environment key to an endpoint the caller chooses, which the README documents as intentional. - Speech copies do run, but only reach Microsoft's Speech hosts, because the region accepts only letters and digits. A new Speech converter created without a key already uses the server's credentials. The one case this closes: a source configured in Python with a token provider for a non-default identity. A copy could change the resource ID and region to use that identity on another Speech resource it can access.
So this reads as a deliberate rule ("a copy always uses its source's connection") rather than a fix for an existing leak. Could the description and the [BREAKING] rationale say that?
| COPY_LOCKED_PARAMETER_NAMES: ClassVar[frozenset[str]] = frozenset( | ||
| {"endpoint", "auth_mode", "api_key", "headers", "httpx_client_kwargs"} | ||
| ) |
There was a problem hiding this comment.
Given that target copies with these overrides are only built for a dry run, does the target half belong in this PR? The only target copies that send are temperature bindings, and the open #3506 stops creating them in favor of a per-request setting (it keeps only replay of saved ones). Unless something that sends target copies with these overrides is planned, I'd leave the target lock out, along with its test changes (including the #3809 update), and add it with that consumer.
| #: Constructor parameters that choose where requests go or how they authenticate. A private | ||
| #: copy built from API input keeps its source's values for these. | ||
| COPY_LOCKED_PARAMETER_NAMES: ClassVar[frozenset[str]] = frozenset() |
There was a problem hiding this comment.
Every converter can be copied automatically (constructor capture, base get_reconstruction_parameters), but this list starts empty. A future converter with an endpoint and a key would let copies change the endpoint unless its author remembers to declare it, and nothing in CI would catch that.
Could we add one test: every copyable class with a parameter from get_sensitive_parameter_names() must list it in COPY_LOCKED_PARAMETER_NAMES? It passes today.
| with pytest.raises(ValidationError, match="including auth_mode"): | ||
| CreateTargetRequest(type="OpenAIChatTarget", source=source, register=False, auth_mode=auth_mode) |
There was a problem hiding this comment.
auth_mode in source.params is now always rejected, but the CreateTargetRequest validator still says "overrides, including auth_mode, in source.params". A caller who follows that 422 gets a 400. Could we update the message (for example, "a copy uses its source's auth mode") and rename this test?
| server. It does not change the source. A copy also keeps the settings that | ||
| choose where requests go and how they sign in, such as `endpoint`, `auth_mode`, | ||
| `api_key` and `headers` for OpenAI targets, and the region, key and resource ID | ||
| for the Azure Speech converters; overriding one of these is an error. Such | ||
| parameters report `copy_locked: true`, and components declare them in | ||
| `COPY_LOCKED_PARAMETER_NAMES`. Missing, changed, or ambiguous sources |
There was a problem hiding this comment.
Could we add this rule to the restrictions in the backend README's Input Validation and Trust Boundary section, and state exactly what it guarantees? A copy keeps its source's connection, but creating a new target can still send server keys to an endpoint the caller chooses.
…opyable class locks its credentials
Description
Problem. A private copy is built from a registered target or converter through the API:
sourceonPOST /api/targetsandPOST /api/converters, converter specs in previews, and converter settings in a technique edit. Today a copy can set its ownendpoint,auth_mode,api_keyorheaders, or for the Azure Speech converters its region, key or resource ID, while still using the source's credential. So a key set up for one endpoint can be used somewhere else.Change.
COPY_LOCKED_PARAMETER_NAMES:endpoint,auth_mode,api_key,headersandhttpx_client_kwargsonOpenAITarget(so every OpenAI target), andazure_speech_region,azure_speech_keyandazure_speech_resource_idon both Speech converters. The parameter catalog marks them withcopy_locked.recreate_instancerefuses them for API input with any value, even the source's own, so a copy can't be used to check a guess at the stored key. The 400 names the fields.What stays the same. Settings like temperature, model, voice, and the target a converter uses can still change on a copy, since each of those keeps every key with its own endpoint. Chat's target copies only change temperature, so they work as before. Creating a target or converter, the create dialog, and copies built in Python (
external_input=False) are unchanged.Breaking. An API call that sets one of these fields on a copy now gets a 400. A saved run whose technique edit set one of these fields on a converter copy can't be resumed or run again, and gets the same 400. To use a different endpoint or credential, create a new target or converter.
#3809 landed first, so this PR also updates its test: clearing
api_keyorheaderson a copy of a no-key target is now refused instead of being ignored.Tests and Documentation
tests/unit/registry/test_converter_registry.py: a copy of either Speech converter can't set the region, key or resource ID, including the source's own region, and can still change the voice; a trusted Python copy can still change the region.tests/unit/backend/test_temporary_components.py: a target copy can't setendpoint(a different one or the source's own),api_key,auth_modeorheaders, and a copy of a no-key target can't clear its key or headers.tests/unit/backend/test_converter_service.py: the same refusal through converter create (registered and not) and preview, nothing gets registered, and a copy's editable settings leave out the locked ones. The preview's conversion is mocked.tests/unit/backend/test_technique_edit_service.py: the same refusal for converter settings in a technique edit.tests/unit/registry/test_resolution.py,tests/unit/models/test_parameter.py:copy_lockedcomes from the class's list and is serialized.frontend/src/components/Registry/CreateConverterDialog.test.tsx: editing a copy hides the locked field and sends only the changed voice, settings carried over from before lose the locked field, and creating a converter still shows it.doc/code/registry/0_registry.md: copies keep these settings, and how a class declares them.pytest -n 4 --dist=loadfile tests/unit/backend tests/unit/registry tests/unit/models tests/unit/setup tests/unit/converter/test_azure_speech_converter.py tests/unit/converter/test_azure_speech_text_converter.py tests/unit/prompt_target/target/test_openai_chat_target.py tests/unit/prompt_target/target/test_openai_target_auth.py: 8,142 passed, 9 skipped, 4 xfailed.npx jest src/components/Registry/CreateConverterDialog.test.tsx: 92 passed.npx tsc --noEmit: passed. ESLint on the changed frontend files: passed. pre-commit (ruff, ty) on the changed files: passed.No JupyText changes.