[ARCH-327] Address security comments. 2 by pavel-raykov · Pull Request #1760 · smartcontractkit/chainlink-common

pavel-raykov · 2026-01-08T13:48:19Z

No description provided.

github-actions · 2026-01-08T13:49:25Z

✅ API Diff Results - No breaking changes

Copilot

Pull request overview

This PR addresses security concerns by replacing a third-party atomic file writing library with an internal implementation that provides explicit control over file permissions. The changes remove the dependency on github.com/natefinch/atomic and introduce a custom atomic file writing solution that ensures sensitive keystore files are created with secure permissions (0600).

Key Changes:

Implemented custom atomic file writing functionality with explicit permission control
Removed external dependency github.com/natefinch/atomic from the project
Added test coverage for the new atomic file writing implementation

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
keystore/atomicfile/write.go	New atomic file writing implementation with configurable file permissions
keystore/atomicfile/write_test.go	Test coverage for the atomic file write functionality
keystore/file.go	Updated to use internal atomic file writer with explicit 0600 permissions
keystore/go.mod	Removed `github.com/natefinch/atomic` dependency
keystore/internal/raw_test.go	Added test assertion for hexadecimal formatting of raw data

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

connorwstein · 2026-01-08T17:37:00Z

+// WriteFile atomically writes the contents of r to the specified filepath with the given mode.
+// This is a copy of https://github.com/natefinch/atomic/blob/master/atomic.go with minor modifications allowing
+// to set mode of the written file. If the file already exists, its mode is preserved.
+func WriteFile(filename string, r io.Reader, mode os.FileMode) (err error) {


I noticed there's an options based here as well https://github.com/natefinch/atomic/pull/18/files - seems slightly better and we could just use it directly?

Yeah if we can, but we shouldn't import a reference that can be deleted.

yeah sorry I meant just copy paste the solution there, not depend on it

Unfortunately, we cannot just copy paste solution here - we still need 1. to fix error output/handling (to make the linter happy), 2. use os.Rename instead of cross-platform Replace. Also, it seems that the copy is just https://github.com/sdassow 's PR - why would we trust it to be better than the current implementation?

I'm not trusting it to be better, I'm saying the options with sane defaults approach implemented there is slightly better. Its a nit though, doesn't matter we can proceed with this

thanks, I have moved it to internal for now

* Minor. * Minor. * Minor. * Minor. * Minor. * Minor. * Minor. --------- Co-authored-by: Connor Stein <connor.stein2@gmail.com>

pavel-raykov had a problem deploying to integration January 8, 2026 13:48 — with GitHub Actions Error

pavel-raykov temporarily deployed to integration January 8, 2026 13:50 — with GitHub Actions Inactive

pavel-raykov temporarily deployed to integration January 8, 2026 14:00 — with GitHub Actions Inactive

pavel-raykov marked this pull request as ready for review January 8, 2026 14:15

pavel-raykov requested a review from a team as a code owner January 8, 2026 14:15

Copilot AI review requested due to automatic review settings January 8, 2026 14:15

pavel-raykov requested a review from a team as a code owner January 8, 2026 14:15

product-security-plaid-production Bot requested review from CL-Andrew and krehermann January 8, 2026 14:15

pavel-raykov marked this pull request as draft January 8, 2026 14:16

Copilot AI reviewed Jan 8, 2026

View reviewed changes

Comment thread keystore/internal/atomicfile/write_test.go

Comment thread keystore/internal/atomicfile/write.go

Comment thread keystore/internal/atomicfile/write.go

Comment thread keystore/internal/atomicfile/write.go

pavel-raykov had a problem deploying to integration January 8, 2026 14:17 — with GitHub Actions Error

pavel-raykov temporarily deployed to integration January 8, 2026 14:21 — with GitHub Actions Inactive

pavel-raykov marked this pull request as ready for review January 8, 2026 14:26

pavel-raykov requested review from connorwstein, jmank88 and timweri January 8, 2026 14:26

connorwstein reviewed Jan 8, 2026

View reviewed changes

connorwstein previously approved these changes Jan 9, 2026

View reviewed changes

pavel-raykov dismissed connorwstein’s stale review via 3e2482c January 9, 2026 15:41

pavel-raykov had a problem deploying to integration January 9, 2026 15:41 — with GitHub Actions Error

pavel-raykov requested a review from connorwstein January 9, 2026 15:43

pavel-raykov added 7 commits January 9, 2026 16:44

Minor.

115cb30

Minor.

26edf93

Minor.

09f4936

Minor.

9ba1b8a

Minor.

eed7b14

Minor.

6332e33

Minor.

ddc5dce

pavel-raykov force-pushed the address-sec2 branch from 3e2482c to ddc5dce Compare January 9, 2026 15:44

pavel-raykov temporarily deployed to integration January 9, 2026 15:45 — with GitHub Actions Inactive

Merge branch 'main' into address-sec2

1d9018d

connorwstein approved these changes Jan 9, 2026

View reviewed changes

connorwstein temporarily deployed to integration January 9, 2026 17:24 — with GitHub Actions Inactive

timweri approved these changes Jan 9, 2026

View reviewed changes

jmank88 approved these changes Jan 9, 2026

View reviewed changes

connorwstein added this pull request to the merge queue Jan 9, 2026

Merged via the queue into main with commit 6efadaa Jan 9, 2026
35 of 36 checks passed

connorwstein deleted the address-sec2 branch January 9, 2026 20:37

cawthorne pushed a commit that referenced this pull request Jan 13, 2026

[ARCH-327] Address security comments. 2 (#1760)

d01ca26

* Minor. * Minor. * Minor. * Minor. * Minor. * Minor. * Minor. --------- Co-authored-by: Connor Stein <connor.stein2@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ARCH-327] Address security comments. 2#1760

[ARCH-327] Address security comments. 2#1760
connorwstein merged 8 commits intomainfrom
address-sec2

pavel-raykov commented Jan 8, 2026

Uh oh!

github-actions Bot commented Jan 8, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

connorwstein Jan 8, 2026

Uh oh!

jmank88 Jan 8, 2026

Uh oh!

connorwstein Jan 8, 2026

Uh oh!

pavel-raykov Jan 9, 2026

Uh oh!

connorwstein Jan 9, 2026

Uh oh!

pavel-raykov Jan 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

pavel-raykov commented Jan 8, 2026

Uh oh!

github-actions Bot commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ API Diff Results - No breaking changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

connorwstein Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

jmank88 Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

connorwstein Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

pavel-raykov Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

connorwstein Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

pavel-raykov Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions Bot commented Jan 8, 2026 •

edited

Loading