Sanitize paths (#1132)

* Normalize and encodeURI paths

* Test path sanitization

* Move RepositoryService.test.js

* Don't encodeURI and add e2e test

* Remove .only in test

* Remove ../ even after normalize

* Add tests for not saving path-translation

src/server/services/RepositoryService.ts

@@ -521,18 +521,35 @@ export class RepositoryService implements IRepositoryService {
     address: string,
     sources: StringMap
   ) {
+    const pathTranslation: StringMap = {};
     for (const sourcePath in sources) {
+      const { sanitizedPath, originalPath } = this.sanitizePath(sourcePath);
+      if (sanitizedPath !== originalPath) {
+        pathTranslation[originalPath] = sanitizedPath;
+      }
       this.save(
         {
           matchQuality,
           chainId,
           address,
           source: true,
-          fileName: this.sanitizePath(sourcePath),
+          fileName: sanitizedPath,
         },
         sources[sourcePath]
       );
     }
+    // Finally save the path translation
+    if (Object.keys(pathTranslation).length === 0) return;
+    this.save(
+      {
+        matchQuality,
+        chainId,
+        address,
+        source: false,
+        fileName: "path-translation.json",
+      },
+      JSON.stringify(pathTranslation)
+    );
   }
 
   private storeJSON(
@@ -609,24 +626,26 @@ export class RepositoryService implements IRepositoryService {
       await this.ipfsClient.files.cp(addResult.cid, mfsPath, { parents: true });
     }
   }
-  // This needs to be removed at some point https://github.com/ethereum/sourcify/issues/515
-  private sanitizePath(originalPath: string): string {
-    const parsedPath = path.parse(originalPath);
+  private sanitizePath(originalPath: string) {
+    // Clean ../ and ./ from the path. Also collapse multiple slashes into one.
+    let sanitizedPath = path.normalize(originalPath);
+
+    // If there are no upper folders to traverse, path.normalize will keep ../ parts. Need to remove any of those.
+    const parsedPath = path.parse(sanitizedPath);
     const sanitizedDir = parsedPath.dir
       .split(path.sep)
       .filter((segment) => segment !== "..")
-      .join(path.sep)
-      .replace(/[^a-z0-9_./-]/gim, "_")
-      .replace(/(^|\/)[.]+($|\/)/, "_");
+      .join(path.sep);
 
     // Force absolute paths to be relative
     if (parsedPath.root) {
-      parsedPath.root = "";
       parsedPath.dir = sanitizedDir.slice(parsedPath.root.length);
+      parsedPath.root = "";
     } else {
       parsedPath.dir = sanitizedDir;
     }
 
-    return path.format(parsedPath);
+    sanitizedPath = path.format(parsedPath);
+    return { sanitizedPath, originalPath };
   }
 }

test/RepositoryService.test.js

@@ -0,0 +1,77 @@
+const { expect } = require("chai");
+const {
+  RepositoryService,
+} = require("../dist/server/services/RepositoryService");
+
+describe("RepositoryService", () => {
+  const instance = new RepositoryService("./dist/data/mock-repository");
+
+  describe("sanitizePath function", () => {
+    it("should remove directory traversal sequences", () => {
+      const result = instance.sanitizePath("some/path/../to/file.txt");
+      expect(result.sanitizedPath).to.equal("some/to/file.txt");
+      expect(result.originalPath).to.equal("some/path/../to/file.txt");
+    });
+
+    it("should return the original path unchanged", () => {
+      const result = instance.sanitizePath("some/path/to/file.txt");
+      expect(result.sanitizedPath).to.equal("some/path/to/file.txt");
+      expect(result.originalPath).to.equal("some/path/to/file.txt");
+    });
+
+    it("should convert absolute paths to relative", () => {
+      const result = instance.sanitizePath("/absolute/path/to/file.txt");
+      expect(result.sanitizedPath).to.equal("absolute/path/to/file.txt");
+      expect(result.originalPath).to.equal("/absolute/path/to/file.txt");
+    });
+
+    it("should not keep any .. even if there are no upper directories left", () => {
+      const result = instance.sanitizePath("path/../../../../to/file.txt");
+      expect(result.sanitizedPath).to.equal("to/file.txt");
+      expect(result.originalPath).to.equal("path/../../../../to/file.txt");
+    });
+
+    it("should sanitize a path containing localhost and directory traversal sequences", () => {
+      const result = instance.sanitizePath(
+        "localhost/../Users/pc/workspace/remixsrc/openzeppelin-contracts/IOSB/token/ERC20/ERC20Pausable.sol"
+      );
+      expect(result.sanitizedPath).to.equal(
+        "Users/pc/workspace/remixsrc/openzeppelin-contracts/IOSB/token/ERC20/ERC20Pausable.sol"
+      );
+      expect(result.originalPath).to.equal(
+        "localhost/../Users/pc/workspace/remixsrc/openzeppelin-contracts/IOSB/token/ERC20/ERC20Pausable.sol"
+      );
+    });
+
+    it("should not modify a file name containing '..'", () => {
+      const result = instance.sanitizePath("myToken..sol");
+      expect(result.sanitizedPath).to.equal("myToken..sol");
+      expect(result.originalPath).to.equal("myToken..sol");
+    });
+
+    it("should sanitize a URL path containing directory traversal sequences", () => {
+      const result = instance.sanitizePath(
+        "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/../../utils/Context.sol"
+      );
+      expect(result.sanitizedPath).to.equal(
+        "https:/github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Context.sol"
+      );
+      expect(result.originalPath).to.equal(
+        "https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/../../utils/Context.sol"
+      );
+    });
+    it("should not change special characters in the path", () => {
+      const result = instance.sanitizePath(
+        "project:/contracts/a`~!@#$%^&*()-=_+[]{}|\\;:'\",<>?ÿø±ö«»¿ð�~K�~X��[email protected]"
+      );
+      expect(result.sanitizedPath).to.equal(
+        "project:/contracts/a`~!@#$%^&*()-=_+[]{}|\\;:'\",<>?ÿø±ö«»¿ð�~K�~X��[email protected]"
+      );
+      expect(result.originalPath).to.equal(
+        "project:/contracts/a`~!@#$%^&*()-=_+[]{}|\\;:'\",<>?ÿø±ö«»¿ð�~K�~X��[email protected]"
+      );
+    });
+  });
+
+  // ... more tests for other methods of RepositoryService
+});

test/server.js

@@ -45,7 +45,7 @@ const {
   callWithAccessToken,
 } = require("./helpers/helpers");
 const { deployFromAbiAndBytecode } = require("./helpers/helpers");
-const { JsonRpcProvider, Network } = require("ethers");
+const { JsonRpcProvider, Network, id: keccak256str } = require("ethers");
 const { LOCAL_CHAINS } = require("../dist/sourcify-chains");
 chai.use(chaiHttp);
 
@@ -945,7 +945,6 @@ describe("Server", function () {
             defaultContractChain,
             contractAddress,
             "sources",
-            "..contracts",
             "Storage.sol"
           )
         );
@@ -1711,6 +1710,155 @@ describe("Server", function () {
       });
     });
   });
+  describe("E2E test path sanitization", async function () {
+    it("should verify a contract with paths containing misc. chars, save the path translation, and be able access the file over the API", async () => {
+      const sanitizeArtifact = require("./testcontracts/path-sanitization/ERC20.json");
+      const sanitizeMetadata = require("./testcontracts/path-sanitization/metadata.json");
+      // read all files under test/testcontracts/path-sanitization/sources/ and put them in an object
+      const sanitizeSourcesObj = {};
+      fs.readdirSync(
+        path.join("test", "testcontracts", "path-sanitization", "sources")
+      ).forEach(
+        (fileName) =>
+          (sanitizeSourcesObj[fileName] = fs.readFileSync(
+            path.join(
+              "test",
+              "testcontracts",
+              "path-sanitization",
+              "sources",
+              fileName
+            )
+          ))
+      );
+
+      const sanitizeMetadataBuffer = Buffer.from(
+        JSON.stringify(sanitizeMetadata)
+      );
+
+      const toBeSanitizedContractAddress = await deployFromAbiAndBytecode(
+        localSigner,
+        sanitizeArtifact.abi,
+        sanitizeArtifact.bytecode,
+        ["TestToken", "TEST", 1000000000]
+      );
+
+      const verificationResponse = await chai
+        .request(server.app)
+        .post("/")
+        .send({
+          address: toBeSanitizedContractAddress,
+          chain: defaultContractChain,
+          files: {
+            "metadata.json": sanitizeMetadataBuffer.toString(),
+            ...sanitizeSourcesObj,
+          },
+        });
+
+      chai.expect(verificationResponse.status).to.equal(StatusCodes.OK);
+      chai
+        .expect(verificationResponse.body.result[0].status)
+        .to.equal("perfect");
+      const contractSavedPath = path.join(
+        server.repository,
+        "contracts",
+        "full_match",
+        defaultContractChain,
+        toBeSanitizedContractAddress
+      );
+      const pathTranslationPath = path.join(
+        contractSavedPath,
+        "path-translation.json"
+      );
+
+      let pathTranslationJSON;
+      try {
+        pathTranslationJSON = JSON.parse(
+          fs.readFileSync(pathTranslationPath).toString()
+        );
+      } catch (e) {
+        throw new Error(
+          `Path translation file not found at ${pathTranslationPath}`
+        );
+      }
+
+      // Get the contract files from the server
+      const res = await chai
+        .request(server.app)
+        .get(`/files/${defaultContractChain}/${toBeSanitizedContractAddress}`);
+      chai.expect(res.status).to.equal(StatusCodes.OK);
+
+      // The translation path must inlude the new translated path
+      const fetchedContractFiles = res.body;
+      Object.keys(pathTranslationJSON).forEach((originalPath) => {
+        // The metadata must have the original path
+        chai
+          .expect(
+            sanitizeMetadata.sources,
+            `Original path ${originalPath} not found in metadata`
+          )
+          .to.include.key(originalPath);
+        // The path from the server response must be translated
+        const translatedContractObject = fetchedContractFiles.find(
+          (obj) =>
+            obj.path ===
+            path.join(
+              contractSavedPath,
+              "sources",
+              pathTranslationJSON[originalPath]
+            )
+        );
+        chai.expect(translatedContractObject).to.exist;
+        // And the saved file must be the same as in the metadata
+        chai
+          .expect(
+            sanitizeMetadata.sources[originalPath].keccak256,
+            `Keccak of ${originalPath} does not match ${translatedContractObject.path}`
+          )
+          .to.equal(keccak256str(translatedContractObject.content));
+      });
+    });
+
+    it("should not save path translation if the path is not sanitized", async () => {
+      const contractAddress = await deployFromAbiAndBytecode(
+        localSigner,
+        artifact.abi,
+        artifact.bytecode
+      );
+      await chai
+        .request(server.app)
+        .post("/")
+        .send({
+          address: defaultContractAddress,
+          chain: defaultContractChain,
+          files: {
+            "metadata.json": metadataBuffer,
+            "Storage.sol": sourceBuffer,
+          },
+        })
+        .end((err, res) =>
+          assertVerification(
+            err,
+            res,
+            null,
+            defaultContractAddress,
+            defaultContractChain,
+            "perfect"
+          )
+        );
+      const contractSavedPath = path.join(
+        server.repository,
+        "contracts",
+        "full_match",
+        defaultContractChain,
+        contractAddress
+      );
+      const pathTranslationPath = path.join(
+        contractSavedPath,
+        "path-translation.json"
+      );
+      chai.expect(fs.existsSync(pathTranslationPath)).to.be.false;
+    });
+  });
   describe("Verify repository endpoints", function () {
     const agent = chai.request.agent(server.app);
     it("should fetch files of specific address", async function () {