Skip to content

Fix editing connection with sensitive extra field #52403

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jscheffl merged 5 commits into from
Jun 29, 2025
Merged

Fix editing connection with sensitive extra field #52403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ac30af
Handle unchanges json
shubhamraj-git Jun 28, 2025
ca4f141
Merge branch 'main' into issue-52301
shubhamraj-git Jun 28, 2025
b05e72e
Remove the redact from connections
shubhamraj-git Jun 28, 2025
51f9666
Fix the static checks
shubhamraj-git Jun 29, 2025
ef9a493
Merge branch 'main' into issue-52301
shubhamraj-git Jun 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 1 addition & 24 deletions airflow-core/src/airflow/api_fastapi/core_api/datamodels/connections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,12 @@

from __future__ import annotations

import json
from collections import abc
from typing import Annotated

from pydantic import Field, field_validator
from pydantic_core.core_schema import ValidationInfo
from pydantic import Field

from airflow.api_fastapi.core_api.base import BaseModel, StrictBaseModel
from airflow.sdk.execution_time.secrets_masker import redact


# Response Models
Expand All @@ -42,26 +39,6 @@ class ConnectionResponse(BaseModel):
password: str | None
extra: str | None

@field_validator("password", mode="after")
@classmethod
def redact_password(cls, v: str | None, field_info: ValidationInfo) -> str | None:
if v is None:
return None
return str(redact(v, field_info.field_name))

@field_validator("extra", mode="before")
@classmethod
def redact_extra(cls, v: str | None) -> str | None:
if v is None:
return None
try:
extra_dict = json.loads(v)
redacted_dict = redact(extra_dict)
return json.dumps(redacted_dict)
except json.JSONDecodeError:
# we can't redact fields in an unstructured `extra`
return v


class ConnectionCollectionResponse(BaseModel):
"""Connection Collection serializer for responses."""
Expand Down
12 changes: 10 additions & 2 deletions airflow-core/src/airflow/ui/src/pages/Connections/ConnectionForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const ConnectionForm = ({
const { conf: extra, setConf } = useParamStore();
const {
control,
formState: { isValid },
formState: { isDirty, isValid },
handleSubmit,
reset,
watch,
Expand Down Expand Up @@ -94,6 +94,14 @@ const ConnectionForm = ({
mutateConnection(data);
};

const hasChanges = () => {
Copy link
Contributor

@bbovenzi bbovenzi Jun 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Nit. functions should start with a verb.

But we don't even need a function here.

const hasChanges = isDirty || (JSON.stringify(JSON.parse(extra)) !== JSON.stringify(JSON.parse(initialConnection.extra)))

Copy link
Contributor

@bbovenzi bbovenzi Jun 30, 2025

Choose a reason for hiding this comment

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

And whats the point of parsing first?

Copy link
Contributor Author

@shubhamraj-git shubhamraj-git Jun 30, 2025

Choose a reason for hiding this comment

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

Actually yes, in current state func shouldn't be, In starting I was doing multiple things and debugging, later should have changed it.
Reason for parsing, A user can add something from cli which would not be in UI formatted way, it should not be treated as different value.

if (isDirty) {
return true;
}

return JSON.stringify(JSON.parse(extra)) !== JSON.stringify(JSON.parse(initialConnection.extra));
};

const validateAndPrettifyJson = (value: string) => {
try {
const parsedJson = JSON.parse(value) as JSON;
Expand Down Expand Up @@ -234,7 +242,7 @@ const ConnectionForm = ({
<Spacer />
<Button
colorPalette="blue"
disabled={Boolean(errors.conf) || formErrors || isPending || !isValid}
disabled={Boolean(errors.conf) || formErrors || isPending || !isValid || !hasChanges()}
onClick={() => void handleSubmit(onSubmit)()}
>
<FiSave /> {translate("formActions.save")}
Expand Down
4 changes: 3 additions & 1 deletion airflow-core/src/airflow/ui/src/queries/useEditConnection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,9 @@ export const useEditConnection = (
const editConnection = (requestBody: ConnectionBody) => {
const updateMask: Array<string> = [];

if (requestBody.extra !== initialConnection.extra) {
if (
JSON.stringify(JSON.parse(requestBody.extra)) !== JSON.stringify(JSON.parse(initialConnection.extra))
) {
updateMask.push("extra");
}
if (requestBody.conn_type !== initialConnection.conn_type) {
Expand Down
81 changes: 12 additions & 69 deletions airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,35 +159,6 @@ def test_get_should_respond_200_with_extra(self, test_client, session):
assert body["conn_type"] == TEST_CONN_TYPE
assert body["extra"] == '{"extra_key": "extra_value"}'

@pytest.mark.enable_redact
def test_get_should_respond_200_with_extra_redacted(self, test_client, session):
self.create_connection()
connection = session.query(Connection).first()
connection.extra = '{"password": "test-password"}'
session.commit()
response = test_client.get(f"/connections/{TEST_CONN_ID}")
assert response.status_code == 200
body = response.json()
assert body["connection_id"] == TEST_CONN_ID
assert body["conn_type"] == TEST_CONN_TYPE
assert body["extra"] == '{"password": "***"}'

@pytest.mark.enable_redact
def test_get_should_not_overmask_short_password_value_in_extra(self, test_client, session):
connection = Connection(
conn_id=TEST_CONN_ID, conn_type="generic", login="a", password="a", extra='{"key": "value"}'
)
session.add(connection)
session.commit()

response = test_client.get(f"/connections/{TEST_CONN_ID}")
assert response.status_code == 200
body = response.json()
assert body["connection_id"] == TEST_CONN_ID
assert body["conn_type"] == "generic"
assert body["login"] == "a"
assert body["extra"] == '{"key": "value"}'


class TestGetConnections(TestConnectionEndpoint):
@pytest.mark.parametrize(
Expand Down Expand Up @@ -309,7 +280,6 @@ def test_post_should_respond_already_exist(self, test_client, body):
assert "detail" in response_json
assert list(response_json["detail"].keys()) == ["reason", "statement", "orig_error", "message"]

@pytest.mark.enable_redact
@pytest.mark.parametrize(
"body, expected_response",
[
Expand All @@ -322,21 +292,7 @@ def test_post_should_respond_already_exist(self, test_client, body):
"extra": None,
"host": None,
"login": None,
"password": "***",
"port": None,
"schema": None,
},
),
(
{"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "password": "?>@#+!_%()#"},
{
"connection_id": TEST_CONN_ID,
"conn_type": TEST_CONN_TYPE,
"description": None,
"extra": None,
"host": None,
"login": None,
"password": "***",
"password": "test-password",
"port": None,
"schema": None,
},
Expand All @@ -352,21 +308,23 @@ def test_post_should_respond_already_exist(self, test_client, body):
"connection_id": TEST_CONN_ID,
"conn_type": TEST_CONN_TYPE,
"description": None,
"extra": '{"password": "***"}',
"extra": '{"password": "test-password"}',
"host": None,
"login": None,
"password": "***",
"password": "A!rF|0wi$aw3s0m3",
"port": None,
"schema": None,
},
),
],
)
def test_post_should_response_201_redacted_password(self, test_client, body, expected_response, session):
def test_post_should_response_201_password_not_masked(
self, test_client, body, expected_response, session
):
response = test_client.post("/connections", json=body)
assert response.status_code == 201
assert response.json() == expected_response
_check_last_log(session, dag_id=None, event="post_connection", logical_date=None, check_masked=True)
_check_last_log(session, dag_id=None, event="post_connection", logical_date=None)


class TestPatchConnection(TestConnectionEndpoint):
Expand Down Expand Up @@ -776,22 +734,7 @@ def test_patch_should_respond_404(self, test_client, body):
"extra": None,
"host": "some_host_a",
"login": "some_login",
"password": "***",
"port": 8080,
"schema": None,
},
{"update_mask": ["password"]},
),
(
{"connection_id": TEST_CONN_ID, "conn_type": TEST_CONN_TYPE, "password": "?>@#+!_%()#"},
{
"connection_id": TEST_CONN_ID,
"conn_type": TEST_CONN_TYPE,
"description": "some_description_a",
"extra": None,
"host": "some_host_a",
"login": "some_login",
"password": "***",
"password": "test-password",
"port": 8080,
"schema": None,
},
Expand All @@ -808,25 +751,25 @@ def test_patch_should_respond_404(self, test_client, body):
"connection_id": TEST_CONN_ID,
"conn_type": TEST_CONN_TYPE,
"description": "some_description_a",
"extra": '{"password": "***"}',
"extra": '{"password": "test-password"}',
"host": "some_host_a",
"login": "some_login",
"password": "***",
"password": "A!rF|0wi$aw3s0m3",
"port": 8080,
"schema": None,
},
{"update_mask": ["password", "extra"]},
),
],
)
def test_patch_should_response_200_redacted_password(
def test_patch_should_response_200_password_not_masked(
self, test_client, session, body, expected_response, update_mask
):
self.create_connections()
response = test_client.patch(f"/connections/{TEST_CONN_ID}", json=body, params=update_mask)
assert response.status_code == 200
assert response.json() == expected_response
_check_last_log(session, dag_id=None, event="patch_connection", logical_date=None, check_masked=True)
_check_last_log(session, dag_id=None, event="patch_connection", logical_date=None)


class TestConnection(TestConnectionEndpoint):
Expand Down