Re: [PR] Avoid logging Execution API bearer credentials [airflow]
jason810496 merged PR #67059: URL: https://github.com/apache/airflow/pull/67059 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Avoid logging Execution API bearer credentials [airflow]
potiuk commented on PR #67059: URL: https://github.com/apache/airflow/pull/67059#issuecomment-4529556850 @Leondon9 A few things need addressing before review — see our [Pull Request quality criteria](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-quality-criteria). - :x: **CI fails**: `Low dep tests: providers / All-prov:LowestDeps:14:3.10:airbyte...apprise` (and possibly other checks — see the [Checks tab](https://github.com/apache/airflow/pull/67059/checks) for the full list). No rush. --- _Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this [two-stage triage process](https://github.com/apache/airflow/blob/main/contributing-docs/25_maintainer_pr_triage.md#why-the-first-pass-is-automated) so that our maintainers' limited time is spent where it matters most: the conversation with you._ --- Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Avoid logging Execution API bearer credentials [airflow]
Leondon9 commented on code in PR #67059:
URL: https://github.com/apache/airflow/pull/67059#discussion_r3276499289
##
airflow-core/src/airflow/api_fastapi/execution_api/security.py:
##
@@ -124,7 +124,7 @@ async def __call__( # type: ignore[override]
try:
claims = await validator.avalidated_claims(creds.credentials,
dict(self.required_claims))
except Exception as err:
-log.warning("Failed to validate JWT", exc_info=True,
token=creds.credentials)
+log.warning("Failed to validate JWT", exc_info=True)
raise HTTPException(status_code=status.HTTP_403_FORBIDDEN,
detail=f"Invalid auth token: {err}")
Review Comment:
Thanks, that makes sense. I updated the client-facing detail to use a
generic `Invalid auth token` message and kept the detailed exception available
only through the server-side log via `exc_info=True`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Avoid logging Execution API bearer credentials [airflow]
Leondon9 commented on code in PR #67059: URL: https://github.com/apache/airflow/pull/67059#discussion_r3276490984 ## airflow-core/newsfragments/67059.bugfix.rst: ## Review Comment: Thank you for your comments. I removed the fragment. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Avoid logging Execution API bearer credentials [airflow]
Pedrinhonitz commented on code in PR #67059:
URL: https://github.com/apache/airflow/pull/67059#discussion_r3255423815
##
airflow-core/newsfragments/67059.bugfix.rst:
##
Review Comment:
Please revert or remove the fragment
##
airflow-core/src/airflow/api_fastapi/execution_api/security.py:
##
@@ -124,7 +124,7 @@ async def __call__( # type: ignore[override]
try:
claims = await validator.avalidated_claims(creds.credentials,
dict(self.required_claims))
except Exception as err:
-log.warning("Failed to validate JWT", exc_info=True,
token=creds.credentials)
+log.warning("Failed to validate JWT", exc_info=True)
raise HTTPException(status_code=status.HTTP_403_FORBIDDEN,
detail=f"Invalid auth token: {err}")
Review Comment:
Would it make sense for these details to return something more generic?
Without the `err` variable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Avoid logging Execution API bearer credentials [airflow]
jscheffl commented on code in PR #67059: URL: https://github.com/apache/airflow/pull/67059#discussion_r3254844038 ## airflow-core/newsfragments/67059.bugfix.rst: ## Review Comment: Yes, please NO newsfragments for bug fixes. This is for IMPORTANT announcements in releases ONLY. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Avoid logging Execution API bearer credentials [airflow]
Pedrinhonitz commented on code in PR #67059: URL: https://github.com/apache/airflow/pull/67059#discussion_r3254790954 ## airflow-core/newsfragments/67059.bugfix.rst: ## Review Comment: Thank you for your contribution. I believe `new fragment` is for changelogs and I don't think we need it here, right, @jscheffl? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
