LibFs: require a Solidity identifier, bootstrap the generated dir, document the unlink

src/lib/LibFs.sol

@@ -5,31 +5,66 @@ pragma solidity ^0.8.25;
 import {Vm} from "forge-std-1.16.1/src/Vm.sol";
 import {LibCodeGen} from "./LibCodeGen.sol";
 
+/// @dev The directory that generated contract files are written to, relative to
+/// the project root. Consumers commit this directory and import from it by
+/// path, so it is a cross repo contract rather than an internal detail.
+string constant GENERATED_DIR = "src/generated";
+
 /// @title LibFs
 /// @notice A library for file system operations related to code generation.
 /// @dev Uses foundry's Vm cheat codes for file operations. Notably standardizes
 /// the placement and idempotent creation of generated files.
 library LibFs {
     /// @notice Constructs the file path for a contract's generated file.
-    /// @param contractName The name of the contract.
+    ///
+    /// Reverts unless `contractName` is a Solidity identifier, so every path
+    /// this function returns is a direct child of `GENERATED_DIR`. The check is
+    /// here rather than at the write because the path is what carries the name
+    /// out of this library: a caller that takes the returned path and does its
+    /// own IO with it gets the same confinement `buildFileForContract` does,
+    /// and there is no name for which this library produces a path at all
+    /// without producing a safe one.
+    ///
+    /// An accepted name is interpolated verbatim, so it reaches the path byte
+    /// for byte and is never quoted, escaped, trimmed, case folded or
+    /// truncated.
+    /// @param contractName The name of the contract, interpolated verbatim.
     /// @return The file path as a string.
     function pathForContract(string memory contractName) internal pure returns (string memory) {
-        return string.concat("src/generated/", contractName, ".sol");
+        LibCodeGen.requireContractName(contractName);
+        return string.concat(GENERATED_DIR, "/", contractName, ".sol");
     }
 
-    /// @notice Builds a file for a generated contract, removing any existing
-    /// file at the same path. This ensures idempotent file generation but will
-    /// delete any manual changes to the generated file, or existing file at
-    /// that path. The prefix and bytecode hash constant are always included,
-    /// further content is provided in the body parameter, which is expected to
-    /// be generated by `LibCodeGen` by the caller.
+    /// @notice Builds a file for a generated contract at
+    /// `pathForContract(contractName)`.
+    ///
+    /// `contractName` must be a Solidity identifier, which `pathForContract`
+    /// requires of every path it returns, so the file is always a direct child
+    /// of `GENERATED_DIR` and a rejected name reverts before any cheatcode is
+    /// reached.
+    ///
+    /// `GENERATED_DIR` is created if it does not exist, so the first generation
+    /// in a repo does not need it committed already.
+    ///
+    /// Anything already at the path is unlinked before the write, so a symlink
+    /// there is replaced by a regular file rather than written through to its
+    /// target, and the path does not exist between the unlink and the write.
+    /// Any manual changes to the generated file, or any other existing file at
+    /// that path, are lost.
+    ///
+    /// The whole file is written on every call, so the same arguments always
+    /// produce the same bytes. The prefix and bytecode hash constant are always
+    /// included, further content is provided in the body parameter, which is
+    /// expected to be generated by `LibCodeGen` by the caller.
     /// @param vm The Vm instance for file operations.
     /// @param instance The contract instance whose bytecode hash is to be
     /// included.
     /// @param contractName The name of the contract.
     /// @param body The body of the contract file to be written.
     function buildFileForContract(Vm vm, address instance, string memory contractName, string memory body) internal {
         string memory path = pathForContract(contractName);
+        //forge-lint: disable-next-line(unsafe-cheatcode)
+        vm.createDir(GENERATED_DIR, true);
         if (vm.exists(path)) {
             //forge-lint: disable-next-line(unsafe-cheatcode)
             vm.removeFile(path);

test/concrete/LibFsExternal.sol

@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: LicenseRef-DCL-1.0
+// SPDX-FileCopyrightText: Copyright (c) 2020 Rain Open Source Software Ltd
+pragma solidity =0.8.25;
+
+import {Vm} from "forge-std-1.16.1/src/Vm.sol";
+import {LibFs} from "src/lib/LibFs.sol";
+
+/// @title LibFsExternal
+/// Puts `LibFs` behind a call frame. `vm.expectRevert` needs one, and the
+/// library functions are internal so they are inlined into whatever calls them.
+contract LibFsExternal {
+    function buildFileForContract(Vm vm, address instance, string memory contractName, string memory body) external {
+        LibFs.buildFileForContract(vm, instance, contractName, body);
+    }
+
+    function pathForContract(string memory contractName) external pure returns (string memory) {
+        return LibFs.pathForContract(contractName);
+    }
+}

test/lib/LibCodeGen.requireContractName.t.sol

@@ -4,6 +4,7 @@ pragma solidity =0.8.25;
 
 import {Test} from "forge-std-1.16.1/src/Test.sol";
 import {LibCodeGen, InvalidContractName} from "src/lib/LibCodeGen.sol";
+import {LibCodeGenSlow, SLOW_HEAD_ALPHABET, SLOW_TAIL_ALPHABET} from "test/lib/LibCodeGenSlow.sol";
 
 /// @title LibCodeGenRequireContractNameTest
 /// @notice `requireContractName` is what stands between a caller supplied
@@ -167,4 +168,69 @@ contract LibCodeGenRequireContractNameTest is Test {
             }
         } catch {}
     }
+
+    /// Exhaustive over the leading byte: all 256 of them, accepted exactly when
+    /// the byte is in the head alphabet. Nothing about the boundaries of the
+    /// accepted ranges is left to a chosen example, and the oracle is the
+    /// alphabet written out character by character rather than the same range
+    /// arithmetic the library uses.
+    function testRequireContractNameEveryLeadingByte() external {
+        for (uint256 i = 0; i < 256; i++) {
+            string memory name = string(bytes.concat(bytes1(uint8(i))));
+            if (LibCodeGenSlow.containsSlow(SLOW_HEAD_ALPHABET, bytes1(uint8(i)))) {
+                assertAccepted(name);
+            } else {
+                assertRejected(name);
+            }
+        }
+    }
+
+    /// Exhaustive over the trailing byte, behind a leading byte that is itself
+    /// accepted. The digits separate this from the leading case: they are
+    /// accepted here and rejected there.
+    function testRequireContractNameEveryTrailingByte() external {
+        for (uint256 i = 0; i < 256; i++) {
+            string memory name = string(bytes.concat(bytes("A"), bytes1(uint8(i))));
+            if (LibCodeGenSlow.containsSlow(SLOW_TAIL_ALPHABET, bytes1(uint8(i)))) {
+                assertAccepted(name);
+            } else {
+                assertRejected(name);
+            }
+        }
+    }
+
+    /// Over arbitrary names of arbitrary length, acceptance agrees with the
+    /// reference alphabet exactly. The reference is spelled out character by
+    /// character, so this fails if either end of any range moves, rather than
+    /// following the library the way an inlined copy of its own arithmetic
+    /// would.
+    function testRequireContractNameMatchesAlphabet(bytes memory nameBytes) external {
+        string memory name = string(nameBytes);
+        if (LibCodeGenSlow.isContractNameSlow(name)) {
+            assertAccepted(name);
+        } else {
+            assertRejected(name);
+        }
+    }
+
+    /// Names built to be identifiers are accepted at any length and across the
+    /// whole alphabet. Fuzzing a name directly essentially never produces an
+    /// identifier, so without constructing them the accepted half of the domain
+    /// is never exercised at all and a check that rejected everything would
+    /// still pass.
+    function testRequireContractNameAcceptsGeneratedIdentifiers(bytes memory seed) external view {
+        string memory name = LibCodeGenSlow.nameFromSeedSlow(seed);
+        assertTrue(bytes(name).length > 0, "generated an empty name");
+        assertAccepted(name);
+    }
+
+    /// A single byte outside the alphabet is enough to reject a name that is
+    /// otherwise an identifier, wherever in the name it sits. A check that only
+    /// looked at the first or the last character would pass this.
+    function testRequireContractNameRejectsOneBadByte(bytes memory seed, uint256 position, uint8 badByte) external {
+        bytes memory nameBytes = bytes(LibCodeGenSlow.nameFromSeedSlow(seed));
+        vm.assume(!LibCodeGenSlow.containsSlow(SLOW_TAIL_ALPHABET, bytes1(badByte)));
+        nameBytes[position % nameBytes.length] = bytes1(badByte);
+        assertRejected(string(nameBytes));
+    }
 }

test/lib/LibCodeGenSlow.sol

@@ -13,6 +13,16 @@ uint256 constant SLOW_LINE_LENGTH = 120;
 /// indents the value by one `tab_width`, which also defaults to 4.
 string constant SLOW_WRAP = "\n    ";
 
+/// @dev Every character a Solidity identifier may begin with, spelled out one by
+/// one rather than as byte ranges. `LibCodeGen.requireContractName` decides with
+/// range comparisons, so an off by one at either end of a range shows up here as
+/// a disagreement instead of moving both sides at once.
+string constant SLOW_HEAD_ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_$";
+
+/// @dev Every character a Solidity identifier may continue with: the head
+/// alphabet and the decimal digits.
+string constant SLOW_TAIL_ALPHABET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz_$0123456789";
+
 /// @title LibCodeGenSlow
 /// @notice A deliberately naive reference for the constant declarations
 /// `LibCodeGen` emits.
@@ -137,4 +147,50 @@ library LibCodeGenSlow {
         }
         return string(name);
     }
+
+    /// True if `char` appears anywhere in `alphabet`.
+    function containsSlow(string memory alphabet, bytes1 char) internal pure returns (bool) {
+        bytes memory alphabetBytes = bytes(alphabet);
+        for (uint256 i = 0; i < alphabetBytes.length; i++) {
+            if (alphabetBytes[i] == char) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /// True if `name` is a Solidity identifier, decided by membership of the
+    /// written out alphabets rather than by arithmetic.
+    function isContractNameSlow(string memory name) internal pure returns (bool) {
+        bytes memory nameBytes = bytes(name);
+        if (nameBytes.length == 0) {
+            return false;
+        }
+        if (!containsSlow(SLOW_HEAD_ALPHABET, nameBytes[0])) {
+            return false;
+        }
+        for (uint256 i = 1; i < nameBytes.length; i++) {
+            if (!containsSlow(SLOW_TAIL_ALPHABET, nameBytes[i])) {
+                return false;
+            }
+        }
+        return true;
+    }
+
+    /// Folds arbitrary bytes into a name that is a Solidity identifier, so that
+    /// the accepted half of the domain can be fuzzed at all. Random bytes are
+    /// essentially never an identifier, so fuzzing names directly only ever
+    /// exercises rejection, and a property stated over accepted names has to
+    /// build them rather than wait for them.
+    function nameFromSeedSlow(bytes memory seed) internal pure returns (string memory) {
+        bytes memory head = bytes(SLOW_HEAD_ALPHABET);
+        bytes memory tail = bytes(SLOW_TAIL_ALPHABET);
+        uint256 length = seed.length == 0 ? 1 : seed.length;
+        bytes memory name = new bytes(length);
+        name[0] = head[(seed.length == 0 ? 0 : uint256(uint8(seed[0]))) % head.length];
+        for (uint256 i = 1; i < length; i++) {
+            name[i] = tail[uint256(uint8(seed[i])) % tail.length];
+        }
+        return string(name);
+    }
 }

test/lib/LibFs.buildFileForContract.t.sol

@@ -4,8 +4,10 @@ pragma solidity =0.8.25;
 
 import {Test} from "forge-std-1.16.1/src/Test.sol";
 import {LibFs} from "src/lib/LibFs.sol";
-import {LibCodeGen} from "src/lib/LibCodeGen.sol";
+import {LibCodeGen, InvalidContractName} from "src/lib/LibCodeGen.sol";
 import {CodeGennable} from "test/concrete/CodeGennable.sol";
+import {LibFsExternal} from "test/concrete/LibFsExternal.sol";
+import {LibCodeGenSlow} from "test/lib/LibCodeGenSlow.sol";
 
 /// @title LibFsBuildFileForContractTest
 /// @notice `buildFileForContract` is the only thing in this repo that touches
@@ -20,6 +22,14 @@ import {CodeGennable} from "test/concrete/CodeGennable.sol";
 /// consumers have the literal committed in their repos, so the literal is the
 /// oracle.
 contract LibFsBuildFileForContractTest is Test {
+    /// `vm.expectRevert` needs a call frame, and `buildFileForContract` is an
+    /// internal library function that is inlined into its caller.
+    LibFsExternal internal immutable iExternal;
+
+    constructor() {
+        iExternal = new LibFsExternal();
+    }
+
     /// Every test writes under `src/generated/`, which is a committed directory
     /// in this repo. Each test owns a distinct name so parallel suites cannot
     /// collide, none of them is `CodeGennable` (the committed artifact), and
@@ -234,4 +244,71 @@ contract LibFsBuildFileForContractTest is Test {
         assertEq(fromOther, expectedFile(other, ""));
         cleanup(name);
     }
+
+    /// Removes whatever is at `path`, so that a test asserting nothing was
+    /// written there establishes its own precondition rather than assuming one.
+    function cleanupPath(string memory path) internal {
+        if (vm.exists(path)) {
+            if (vm.isDir(path)) {
+                vm.removeDir(path, true);
+            } else {
+                vm.removeFile(path);
+            }
+        }
+    }
+
+    /// A name that is not a Solidity identifier gets no path from
+    /// `pathForContract`, and `buildFileForContract` asks for the path before it
+    /// reaches a cheatcode, so the refusal arrives before anything is written.
+    /// That the path is refused at all is asserted in `LibFsTest`; what is
+    /// asserted here is that the write inherits it, and that nothing lands on
+    /// disk when it does.
+    function assertNameRejected(string memory contractName) internal {
+        vm.expectRevert(abi.encodeWithSelector(InvalidContractName.selector, contractName));
+        iExternal.buildFileForContract(vm, address(this), contractName, "");
+    }
+
+    /// The path for an empty name is `src/generated/.sol`: valid generated
+    /// Solidity at a path that no compiler picks up as a contract file and that
+    /// `ls` hides, written by a build that reports success. Refused, and no
+    /// file appears there.
+    function testBuildFileForContractRejectsEmptyName() external {
+        cleanupPath("src/generated/.sol");
+        assertFalse(vm.exists("src/generated/.sol"), "dirty precondition");
+        assertNameRejected("");
+        assertFalse(vm.exists("src/generated/.sol"), "an empty name still wrote a file");
+    }
+
+    /// A separator in the name puts the file in a subdirectory of the generated
+    /// directory, which the `read-write` grant on that directory admits, so the
+    /// refusal has to come from the library.
+    function testBuildFileForContractRejectsSubdirectoryName() external {
+        cleanupPath("src/generated/sub");
+        assertFalse(vm.exists("src/generated/sub"), "dirty precondition");
+        assertNameRejected("sub/LibFsBuildSub");
+        assertFalse(vm.exists("src/generated/sub"), "a separator still created a subdirectory");
+    }
+
+    /// A relative directory reference leaves the generated directory. Under this
+    /// repo's `fs_permissions` the write is refused anyway, but under the
+    /// `read-write` grant on `.` that consumers commonly write it is not, so the
+    /// refusal has to come from here.
+    function testBuildFileForContractRejectsTraversalName() external {
+        assertNameRejected("..");
+        assertNameRejected("../../ESCAPED");
+    }
+
+    /// The `.sol` extension is appended by the library, so a name that carries
+    /// one would produce `Foo.sol.sol`.
+    function testBuildFileForContractRejectsExtensionInName() external {
+        assertNameRejected("LibFsBuildExtension.sol");
+    }
+
+    /// Every name that is not a Solidity identifier is refused, not just the
+    /// ones named above.
+    function testBuildFileForContractRejectsEveryNonIdentifierName(bytes memory nameBytes) external {
+        string memory contractName = string(nameBytes);
+        vm.assume(!LibCodeGenSlow.isContractNameSlow(contractName));
+        assertNameRejected(contractName);
+    }
 }