Skip to content

fix(sast): fix codeql SAST warning#2933

Merged
jiparis merged 3 commits intochainloop-dev:mainfrom
jiparis:fix-codeql
Mar 24, 2026
Merged

fix(sast): fix codeql SAST warning#2933
jiparis merged 3 commits intochainloop-dev:mainfrom
jiparis:fix-codeql

Conversation

@jiparis
Copy link
Member

@jiparis jiparis commented Mar 24, 2026

Add cookie settings for OAuth to fix CodeQL errors: https://github.com/chainloop-dev/chainloop/actions/runs/23467110462

Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
@jiparis jiparis requested review from Piskoo and migmartri March 24, 2026 08:57
Value: value,
Path: "/",
Expires: time.Now().Add(10 * time.Minute),
HttpOnly: true,
Copy link
Member

Choose a reason for hiding this comment

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

this might break local development, have you tried?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, both CLI and UI auth work perfectly.

Copy link
Member Author

@jiparis jiparis Mar 24, 2026

Choose a reason for hiding this comment

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

image

Copy link
Member Author

Choose a reason for hiding this comment

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

The server response:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

And they are sent back after login:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

But it's strange, as I agree it should fail locally. I'll dig deeper into this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, it seems that only Safari would break. Others consider "localhost" as secure: httpwg/http-extensions#2605

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

1 issue found across 1 file

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="app/controlplane/internal/service/auth.go">

<violation number="1" location="app/controlplane/internal/service/auth.go:443">
P1: Forcing `Secure: true` on OAuth cookies breaks HTTP environments (including local/dev), causing callback cookie reads to fail and abort login.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

jiparis added 2 commits March 24, 2026 12:29
Set HttpOnly and SameSite on OAuth cookies to address CodeQL SAST
findings. Secure flag is conditionally set based on server.Version
to avoid breaking local development over HTTP.

Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
This reverts commit 3774b55.

Signed-off-by: Jose I. Paris <jiparis@chainloop.dev>
@jiparis jiparis merged commit 4ac5faa into chainloop-dev:main Mar 24, 2026
15 checks passed
@jiparis jiparis deleted the fix-codeql branch March 24, 2026 12:03
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.

2 participants