fix(publisher-github): don't sanitize asset names before upload (#3485)

Co-authored-by: Erick Zhao <[email protected]>

Author: dsanders11

packages/publisher/github/package.json

@@ -25,10 +25,13 @@
     "@electron-forge/shared-types": "7.3.0",
     "@octokit/core": "^3.2.4",
     "@octokit/plugin-retry": "^3.0.9",
+    "@octokit/request-error": "^2.0.5",
     "@octokit/rest": "^18.0.11",
     "@octokit/types": "^6.1.2",
+    "chalk": "^4.0.0",
     "debug": "^4.3.1",
     "fs-extra": "^10.0.0",
+    "log-symbols": "^4.0.0",
     "mime-types": "^2.1.25"
   },
   "publishConfig": {

packages/publisher/github/src/PublisherGithub.ts

@@ -1,7 +1,12 @@
+import path from 'node:path';
+
 import { PublisherBase, PublisherOptions } from '@electron-forge/publisher-base';
 import { ForgeMakeResult } from '@electron-forge/shared-types';
+import { RequestError } from '@octokit/request-error';
 import { GetResponseDataTypeFromEndpointMethod } from '@octokit/types';
+import chalk from 'chalk';
 import fs from 'fs-extra';
+import logSymbols from 'log-symbols';
 import mime from 'mime-types';
 
 import { PublisherGitHubConfig } from './Config';
@@ -97,9 +102,10 @@ export default class PublisherGithub extends PublisherBase<PublisherGitHubConfig
               uploaded += 1;
               updateUploadStatus();
             };
-            const artifactName = GitHub.sanitizeName(artifactPath);
+            const artifactName = path.basename(artifactPath);
+            const sanitizedArtifactName = GitHub.sanitizeName(artifactName);
             // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-            const asset = release!.assets.find((item: OctokitReleaseAsset) => item.name === artifactName);
+            const asset = release!.assets.find((item: OctokitReleaseAsset) => item.name === sanitizedArtifactName);
             if (asset !== undefined) {
               if (config.force === true) {
                 await github.getGitHub().repos.deleteReleaseAsset({
@@ -111,21 +117,36 @@ export default class PublisherGithub extends PublisherBase<PublisherGitHubConfig
                 return done();
               }
             }
-            await github.getGitHub().repos.uploadReleaseAsset({
-              owner: config.repository.owner,
-              repo: config.repository.name,
-              // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-              release_id: release!.id,
-              // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-              url: release!.upload_url,
-              // https://github.com/octokit/rest.js/issues/1645
-              data: (await fs.readFile(artifactPath)) as unknown as string,
-              headers: {
-                'content-type': mime.lookup(artifactPath) || 'application/octet-stream',
-                'content-length': (await fs.stat(artifactPath)).size,
-              },
-              name: artifactName,
-            });
+            try {
+              const { data: uploadedAsset } = await github.getGitHub().repos.uploadReleaseAsset({
+                owner: config.repository.owner,
+                repo: config.repository.name,
+                // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+                release_id: release!.id,
+                // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+                url: release!.upload_url,
+                // https://github.com/octokit/rest.js/issues/1645
+                data: (await fs.readFile(artifactPath)) as unknown as string,
+                headers: {
+                  'content-type': mime.lookup(artifactPath) || 'application/octet-stream',
+                  'content-length': (await fs.stat(artifactPath)).size,
+                },
+                name: artifactName,
+              });
+              if (uploadedAsset.name !== sanitizedArtifactName) {
+                // There's definitely a bug with GitHub.sanitizeName
+                console.warn(logSymbols.warning, chalk.yellow(`Expected artifact's name to be '${sanitizedArtifactName}' - got '${uploadedAsset.name}'`));
+              }
+            } catch (err) {
+              // If an asset with that name already exists, it's either a bug with GitHub.sanitizeName
+              // where it did not sanitize the artifact name in the same way as GitHub did, or there
+              // was simply a race condition with uploading artifacts with the same name
+              // eslint-disable-next-line @typescript-eslint/no-explicit-any
+              if (err instanceof RequestError && err.status === 422 && (err.response?.data as any)?.errors?.[0].code === 'already_exists') {
+                console.error(`Asset with name '${artifactName}' already exists - there may be a bug with Forge's GitHub.sanitizeName util`);
+              }
+              throw err;
+            }
             return done();
           })
       );

packages/publisher/github/src/util/github.ts

@@ -48,14 +48,22 @@ export default class GitHub {
     return github;
   }
 
-  // Based on https://docs.github.com/en/rest/releases/assets?apiVersion=2022-11-28#upload-a-release-asset and
-  // https://stackoverflow.com/questions/59081778/rules-for-special-characters-in-github-repository-name
+  // Based on https://github.com/cli/cli/blob/b07f955c23fb54c400b169d39255569e240b324e/pkg/cmd/release/upload/upload.go#L131-L153
   static sanitizeName(name: string): string {
-    return path
-      .basename(name)
-      .replace(/\.+/g, '.')
-      .replace(/^\./g, '')
-      .replace(/\.$/g, '')
-      .replace(/[^\w.-]+/g, '-');
+    return (
+      path
+        .basename(name)
+        // Remove diacritics (e.g. é -> e)
+        .normalize('NFD')
+        .replace(/\p{Diacritic}/gu, '')
+        // Replace special characters with dot
+        .replace(/[^\w_.@+-]+/g, '.')
+        // Replace multiple dots with a single dot
+        .replace(/\.+/g, '.')
+        // Remove leading dot if present
+        .replace(/^\./g, '')
+        // Remove trailing dot if present
+        .replace(/\.$/g, '')
+    );
   }
 }

packages/publisher/github/test/github_spec.ts

@@ -115,8 +115,21 @@ describe('GitHub', () => {
       expect(GitHub.sanitizeName('path/to/foo..bar')).to.equal('foo.bar');
     });
 
-    it('should replace non-alphanumeric, non-hyphen characters with hyphens', () => {
-      expect(GitHub.sanitizeName('path/to/foo%$bar   baz.')).to.equal('foo-bar-baz');
+    it('should replace non-alphanumeric, non-hyphen characters with periods', () => {
+      expect(GitHub.sanitizeName('path/to/foo%$bar   baz.')).to.equal('foo.bar.baz');
+    });
+
+    it('should preserve special symbols', () => {
+      expect(GitHub.sanitizeName('path/to/@foo+bar_')).to.equal('@foo+bar_');
+    });
+
+    it('should preserve hyphens', () => {
+      const name = 'electron-fiddle-0.99.0-full.nupkg';
+      expect(GitHub.sanitizeName(`path/to/${name}`)).to.equal(name);
+    });
+
+    it('should remove diacritics', () => {
+      expect(GitHub.sanitizeName('électron')).to.equal('electron');
     });
   });
 });