Skip to content

Commit e436eb9

Browse files
paoloanznpaoloanzn
committed
(refactor) (openai/gpt-5.5, reviewed F, tested T) align RIK contract quality
1 parent fd25433 commit e436eb9

6 files changed

Lines changed: 257 additions & 69 deletions

File tree

‎contracts/src/RIK.sol‎

Lines changed: 145 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -12,41 +12,54 @@ import {JsonClaim} from "./JsonClaim.sol";
1212
contract RIK is ERC721, Ownable {
1313
using RSA for bytes32;
1414

15+
string private constant _EXPECTED_ISS = "https://token.actions.githubusercontent.com";
16+
1517
struct RSAKey {
1618
bytes modulus;
1719
bytes exponent;
1820
bool active;
1921
}
2022

21-
event KeyAdded(bytes32 indexed kid);
22-
event KeyRevoked(bytes32 indexed kid);
23-
24-
// store valid signing keys from GitHub -> they periodically rotate
25-
mapping(bytes32 => RSAKey) private _keys;
26-
2723
struct Repo {
2824
uint64 githubRepoId; // == tokenId -> kept for clarity
2925
uint64 githubOwnerId;
3026
uint64 registeredAt; // block.timestamp -> truncated at uint64
3127
address registrant; // who called register()
3228
}
3329

34-
// EVM log event used for off-chain indexing
30+
event KeyAdded(bytes32 indexed kid);
31+
event KeyRevoked(bytes32 indexed kid);
3532
event RepoRegistered(uint256 indexed repoId, address indexed registrant, uint64 githubOwnerId, uint64 registeredAt);
3633

37-
mapping(uint256 => Repo) private _repos;
38-
39-
// errors
4034
error AlreadyRegistered(uint256 repoId);
4135
error UnknownKid(bytes32 kid);
4236
error BadJwt();
37+
error TokenExpired();
38+
error TokenNotYetValid();
39+
error NotRegistered(uint256 tokenId);
40+
error RepoIdTooLarge(uint256 repoId);
4341

44-
// pin trusted issuer
45-
// NOTE: make it so contract owner can update this
46-
string public constant EXPECTED_ISS = "https://token.actions.githubusercontent.com";
42+
// store valid signing keys from GitHub; they periodically rotate
43+
mapping(bytes32 => RSAKey) private _keys;
44+
mapping(uint256 => Repo) private _repos;
4745

46+
/**
47+
* @dev Sets the initial registry owner.
48+
*/
4849
constructor(address initialOwner) ERC721("Repository Identity Key", "RIK") Ownable(initialOwner) {}
4950

51+
/**
52+
* @dev Registers a GitHub repository identity and mints its RIK to the caller.
53+
*
54+
* Requirements:
55+
*
56+
* - `kid` must identify an active GitHub Actions signing key.
57+
* - `signature` must be valid for `headerB64.payloadB64`.
58+
* - The JWT payload must match the caller, repository id, owner id, issuer, and active time window.
59+
* - `repoId` must not already be registered and must fit in the stored repository metadata.
60+
*
61+
* Emits a {RepoRegistered} event.
62+
*/
5063
function register(
5164
bytes32 kid,
5265
bytes calldata headerB64,
@@ -55,63 +68,114 @@ contract RIK is ERC721, Ownable {
5568
uint256 repoId,
5669
uint64 githubOwnerId
5770
) external {
71+
address registrant = _msgSender();
5872
_verifyJwt(kid, headerB64, payloadB64, signature);
5973

6074
bytes memory payload = bytes(Base64.decode(string(payloadB64)));
75+
_verifyClaims(payload, registrant, repoId, githubOwnerId);
76+
_verifyActiveWindow(payload);
6177

62-
// verify payload contains the right claim
63-
JsonClaim.requireStringClaim(payload, "aud", Strings.toHexString(uint160(msg.sender), 20));
64-
JsonClaim.requireStringClaim(payload, "repository_id", Strings.toString(repoId));
65-
JsonClaim.requireStringClaim(payload, "repository_owner_id", Strings.toString(uint256(githubOwnerId)));
78+
_register(registrant, repoId, githubOwnerId);
79+
}
6680

67-
// verify issuer
68-
JsonClaim.requireStringClaim(payload, "iss", EXPECTED_ISS);
81+
/**
82+
* @dev Returns the trusted GitHub Actions OIDC issuer.
83+
*/
84+
function expectedIssuer() public pure virtual returns (string memory) {
85+
return _EXPECTED_ISS;
86+
}
6987

70-
// verify if JWT is expired
71-
(uint256 exp_, bool fe) = _readUintClaim(payload, "exp");
72-
(uint256 nbf_, bool fn) = _readUintClaim(payload, "nbf");
73-
if (!fe || !fn) revert JsonClaim.ClaimMissing("exp/nbf");
74-
// JWT expiry must be checked against chain time; small validator timestamp drift is acceptable here.
75-
// forge-lint: disable-next-line(block-timestamp)
76-
if (block.timestamp > exp_) revert("token expired");
88+
/**
89+
* @dev Returns the RIK token id for a GitHub repository id.
90+
*/
91+
function tokenIdOf(uint64 githubRepoId) public pure virtual returns (uint256) {
92+
return uint256(githubRepoId);
93+
}
7794

78-
// JWT expiry must be checked against chain time; small validator timestamp drift is acceptable here.
79-
// forge-lint: disable-next-line(block-timestamp)
80-
if (block.timestamp < nbf_) revert("token not yet valid");
95+
/**
96+
* @dev Returns repository metadata for `tokenId`.
97+
*
98+
* Requirements:
99+
*
100+
* - `tokenId` must be registered.
101+
*/
102+
function repoOf(uint256 tokenId) public view virtual returns (Repo memory) {
103+
if (_ownerOf(tokenId) == address(0)) revert NotRegistered(tokenId);
104+
return _repos[tokenId];
105+
}
106+
107+
/**
108+
* @dev Adds or replaces an active GitHub Actions RSA signing key.
109+
*
110+
* Requirements:
111+
*
112+
* - The caller must be the contract owner.
113+
*
114+
* Emits a {KeyAdded} event.
115+
*/
116+
function addKey(bytes32 kid, bytes calldata n, bytes calldata e) external onlyOwner {
117+
_addKey(kid, n, e);
118+
}
119+
120+
/**
121+
* @dev Revokes a GitHub Actions RSA signing key.
122+
*
123+
* Requirements:
124+
*
125+
* - The caller must be the contract owner.
126+
*
127+
* Emits a {KeyRevoked} event.
128+
*/
129+
function revokeKey(bytes32 kid) external onlyOwner {
130+
_revokeKey(kid);
131+
}
81132

82-
// prevent double registration
133+
/**
134+
* @dev Registers `repoId` to `registrant`.
135+
*
136+
* Requirements:
137+
*
138+
* - `repoId` must not already be registered.
139+
* - `repoId` must fit in the stored repository metadata.
140+
*
141+
* Emits a {RepoRegistered} event.
142+
*/
143+
function _register(address registrant, uint256 repoId, uint64 githubOwnerId) internal virtual {
83144
if (_ownerOf(repoId) != address(0)) revert AlreadyRegistered(repoId);
145+
if (repoId > type(uint64).max) revert RepoIdTooLarge(repoId);
84146

85-
Repo memory r = Repo({
86-
// safe to truncate -> collision is irrelevant
147+
// forge-lint: disable-next-line(unsafe-typecast)
148+
uint64 registeredAt = uint64(block.timestamp);
149+
_repos[repoId] = Repo({
150+
// casting is safe because repoId is bounded above.
87151
// forge-lint: disable-next-line(unsafe-typecast)
88152
githubRepoId: uint64(repoId),
89153
githubOwnerId: githubOwnerId,
90-
registeredAt: uint64(block.timestamp),
91-
registrant: msg.sender
154+
registeredAt: registeredAt,
155+
registrant: registrant
92156
});
93-
_repos[repoId] = r;
94157

95-
emit RepoRegistered(repoId, msg.sender, githubOwnerId, r.registeredAt);
158+
emit RepoRegistered(repoId, registrant, githubOwnerId, registeredAt);
96159

97-
_mint(msg.sender, repoId);
160+
_mint(registrant, repoId);
98161
}
99162

100-
function tokenIdOf(uint64 githubRepoId) external pure returns (uint256) {
101-
return uint256(githubRepoId);
102-
}
103-
104-
function repoOf(uint256 tokenId) external view returns (Repo memory) {
105-
require(_ownerOf(tokenId) != address(0), "not registered");
106-
return _repos[tokenId];
107-
}
108-
109-
function addKey(bytes32 kid, bytes calldata n, bytes calldata e) external onlyOwner {
163+
/**
164+
* @dev Adds or replaces an active GitHub Actions RSA signing key.
165+
*
166+
* Emits a {KeyAdded} event.
167+
*/
168+
function _addKey(bytes32 kid, bytes calldata n, bytes calldata e) internal virtual {
110169
_keys[kid] = RSAKey({modulus: n, exponent: e, active: true});
111170
emit KeyAdded(kid);
112171
}
113172

114-
function revokeKey(bytes32 kid) external onlyOwner {
173+
/**
174+
* @dev Revokes a GitHub Actions RSA signing key.
175+
*
176+
* Emits a {KeyRevoked} event.
177+
*/
178+
function _revokeKey(bytes32 kid) internal virtual {
115179
_keys[kid].active = false;
116180
emit KeyRevoked(kid);
117181
}
@@ -130,6 +194,35 @@ contract RIK is ERC721, Ownable {
130194
if (!RSA.pkcs1Sha256(digest, signature, k.exponent, k.modulus)) revert BadJwt();
131195
}
132196

197+
function _verifyClaims(bytes memory payload, address registrant, uint256 repoId, uint64 githubOwnerId)
198+
internal
199+
pure
200+
{
201+
JsonClaim.requireStringClaim(payload, "aud", Strings.toHexString(uint160(registrant), 20));
202+
JsonClaim.requireStringClaim(payload, "repository_id", Strings.toString(repoId));
203+
JsonClaim.requireStringClaim(payload, "repository_owner_id", Strings.toString(uint256(githubOwnerId)));
204+
JsonClaim.requireStringClaim(payload, "iss", _EXPECTED_ISS);
205+
}
206+
207+
function _verifyActiveWindow(bytes memory payload) internal view {
208+
uint256 exp_ = _requireUintClaim(payload, "exp");
209+
uint256 nbf_ = _requireUintClaim(payload, "nbf");
210+
211+
// JWT validity must be checked against chain time; small validator timestamp drift is acceptable here.
212+
// forge-lint: disable-next-line(block-timestamp)
213+
if (block.timestamp > exp_) revert TokenExpired();
214+
215+
// JWT validity must be checked against chain time; small validator timestamp drift is acceptable here.
216+
// forge-lint: disable-next-line(block-timestamp)
217+
if (block.timestamp < nbf_) revert TokenNotYetValid();
218+
}
219+
220+
function _requireUintClaim(bytes memory payload, string memory key) internal pure returns (uint256 value) {
221+
bool found;
222+
(value, found) = _readUintClaim(payload, key);
223+
if (!found) revert JsonClaim.ClaimMissing(key);
224+
}
225+
133226
function _readUintClaim(bytes memory payload, string memory key) internal pure returns (uint256 value, bool found) {
134227
bytes memory marker = abi.encodePacked('"', bytes(key), '":');
135228
int256 pos = JsonClaim.indexOf(payload, marker);
@@ -139,12 +232,15 @@ contract RIK is ERC721, Ownable {
139232
// casting to uint256 is safe because negative positions returned above.
140233
// forge-lint: disable-next-line(unsafe-typecast)
141234
uint256 i = uint256(pos) + marker.length;
235+
uint256 valueStart = i;
236+
if (i == payload.length) return (0, false);
237+
142238
while (i < payload.length) {
143239
uint8 c = uint8(payload[i]);
144240
if (c < 0x30 || c > 0x39) break; // -> not a digit
145241
value = value * 10 + (c - 0x30);
146242
++i;
147243
}
148-
found = true;
244+
found = i > valueStart;
149245
}
150246
}

‎contracts/test/RIK.t.sol‎

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,10 @@ contract RIK_T is Test {
8181
assertEq(rik.symbol(), "RIK");
8282
}
8383

84+
function test_ExpectedIssuer() public view {
85+
assertEq(rik.expectedIssuer(), "https://token.actions.githubusercontent.com");
86+
}
87+
8488
function test_RegisterArbitraryId() public {
8589
uint256 repo_id = 11112;
8690
uint64 owner_github_id = 11111;
@@ -149,7 +153,7 @@ contract RIK_T is Test {
149153
function test_RepoRevertsForUnregistered() public {
150154
uint256 repo_id = 11112;
151155

152-
vm.expectRevert(bytes("not registered"));
156+
vm.expectRevert(abi.encodeWithSelector(RIK.NotRegistered.selector, repo_id));
153157

154158
rik.repoOf(repo_id);
155159
}
@@ -245,6 +249,18 @@ contract RIK_T is Test {
245249
rik.register(f.kid, f.headerB64, f.payloadB64, f.signature, f.repoId, f.ownerId + 1);
246250
}
247251

252+
function test_RejectsRepoIdTooLarge() public {
253+
uint256 repo_id = uint256(type(uint64).max) + 1;
254+
uint64 github_owner_id = 999;
255+
Fixture memory f = _loadFixture("sample-jwt.json", repo_id, github_owner_id, address(this));
256+
257+
_addKey(f);
258+
259+
vm.expectRevert(abi.encodeWithSelector(RIK.RepoIdTooLarge.selector, repo_id));
260+
261+
_register(f, repo_id, github_owner_id);
262+
}
263+
248264
function test_RejectsWrongIssuer() public {
249265
Fixture memory f = _loadFixture("wrong-issuer-jwt.json");
250266

@@ -256,6 +272,28 @@ contract RIK_T is Test {
256272
rik.register(f.kid, f.headerB64, f.payloadB64, f.signature, f.repoId, f.ownerId);
257273
}
258274

275+
function test_RejectsMissingExp() public {
276+
Fixture memory f = _loadFixture("missing-exp-jwt.json");
277+
278+
_addKey(f);
279+
280+
vm.prank(f.recipient);
281+
vm.expectRevert(abi.encodeWithSelector(JsonClaim.ClaimMissing.selector, "exp"));
282+
283+
rik.register(f.kid, f.headerB64, f.payloadB64, f.signature, f.repoId, f.ownerId);
284+
}
285+
286+
function test_RejectsMissingNbf() public {
287+
Fixture memory f = _loadFixture("missing-nbf-jwt.json");
288+
289+
_addKey(f);
290+
291+
vm.prank(f.recipient);
292+
vm.expectRevert(abi.encodeWithSelector(JsonClaim.ClaimMissing.selector, "nbf"));
293+
294+
rik.register(f.kid, f.headerB64, f.payloadB64, f.signature, f.repoId, f.ownerId);
295+
}
296+
259297
function test_RejectsBadSignature() public {
260298
Fixture memory f = _loadFixture("sample-jwt.json");
261299
bytes memory badSignature = f.signature;
@@ -278,7 +316,7 @@ contract RIK_T is Test {
278316

279317
vm.warp(f.exp + 1);
280318
vm.prank(f.recipient);
281-
vm.expectRevert(bytes("token expired"));
319+
vm.expectRevert(RIK.TokenExpired.selector);
282320

283321
rik.register(f.kid, f.headerB64, f.payloadB64, f.signature, f.repoId, f.ownerId);
284322
}
@@ -290,7 +328,7 @@ contract RIK_T is Test {
290328

291329
vm.warp(f.nbf - 1);
292330
vm.prank(f.recipient);
293-
vm.expectRevert(bytes("token not yet valid"));
331+
vm.expectRevert(RIK.TokenNotYetValid.selector);
294332

295333
rik.register(f.kid, f.headerB64, f.payloadB64, f.signature, f.repoId, f.ownerId);
296334
}

‎contracts/test/fixtures/load-fixture.mjs‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,16 @@ const privateKey = createPrivateKey(TEST_RSA_PRIVATE_KEY);
5656
const publicJwk = createPublicKey(privateKey).export({ format: "jwk" });
5757

5858
const headerB64 = b64url({ alg: "RS256", typ: "JWT", kid: kidText });
59-
const payloadB64 = b64url({
59+
const payload = {
6060
iss,
6161
aud: recipient,
6262
repository_id: String(repoId),
6363
repository_owner_id: String(ownerId),
64-
exp,
65-
nbf,
66-
});
64+
};
65+
if (!fixture.omitExp) payload.exp = exp;
66+
if (!fixture.omitNbf) payload.nbf = nbf;
67+
68+
const payloadB64 = b64url(payload);
6769
const signature = sign("RSA-SHA256", Buffer.from(`${headerB64}.${payloadB64}`), privateKey);
6870

6971
const output = JSON.stringify({

‎contracts/test/fixtures/missing-exp-jwt.json‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
{
2+
"kidText": "kid-001",
3+
"iss": "https://token.actions.githubusercontent.com",
4+
"recipient": "0x00000000000000000000000000000000000A11CE",
5+
"repoId": 11112,
6+
"ownerId": 999,
7+
"exp": 4102444800,
8+
"nbf": 0,
9+
"omitExp": true
10+
}

‎contracts/test/fixtures/missing-nbf-jwt.json‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
{
2+
"kidText": "kid-001",
3+
"iss": "https://token.actions.githubusercontent.com",
4+
"recipient": "0x00000000000000000000000000000000000A11CE",
5+
"repoId": 11112,
6+
"ownerId": 999,
7+
"exp": 4102444800,
8+
"nbf": 0,
9+
"omitNbf": true
10+
}

0 commit comments

Comments
 (0)