Skip to content

feat(codescan): C#/Unity support — scan the language most mobile games are written in - #29

Open
treenod-ollie wants to merge 2 commits into
RevylAI:mainfrom
treenod-ollie:feature/csharp-unity-support
Open

feat(codescan): C#/Unity support — scan the language most mobile games are written in#29
treenod-ollie wants to merge 2 commits into
RevylAI:mainfrom
treenod-ollie:feature/csharp-unity-support

Conversation

@treenod-ollie

@treenod-ollie treenod-ollie commented Aug 6, 2026

Copy link
Copy Markdown

Why

Unity powers a large share of the mobile apps that go through App Review, but greenlight's code scan covers Swift/ObjC/JS/TS only. On a Unity project the flow rules judge the app by its native plugin shims alone, which cuts both ways: false positives (a webview shim's registerDefaults:@{@"UserAgent"...} read as account signup) and blind spots (the actual IAP/login/deletion logic lives in C# the scanner never opens).

We validated this branch against a production Unity mobile game (~13k C# files). Three false positives disappeared, and the C# pass surfaced a real issue the Swift-only scan could not see: PlayerPrefs usage with no NSPrivacyAccessedAPICategoryUserDefaults declaration.

What

C# as a scanned language

  • .cscsharp in both codescan and the privacy scanner
  • generic rules (secrets, external payment, mining, ATT, IPv4, HTTP, placeholder, platform-reference) extended to csharp
  • iap-no-restore learns Unity IAP: UnityEngine.Purchasing, IStoreListener/IDetailedStoreListener/CodelessIAPStoreListener as triggers; IAppleExtensions.RestoreTransactions as the restore anti-pattern
  • DetectClaims gets the same patterns, so the runtime tier sees Unity flows too

Unity-aware privacy scan

  • PlayerPrefs is NSUserDefaults-backed on Apple platforms → counts as the User Defaults required-reason API
  • .NET file-metadata APIs (File.GetLastWriteTime etc.) → File Timestamp category
  • Unity's ATT binding (ATTrackingStatusBinding) and post-build NSUserTrackingUsageDescription injection count as ATT implementations

Unity-generated directory skipping

  • Library/, Temp/, Logs/, obj/, UserSettings/ are engine caches that dwarf Assets/ by an order of magnitude — skipped, but only when ProjectSettings/ProjectSettings.asset proves the root is a Unity project, so a non-Unity repo with a Library/ folder keeps full coverage

False-positive fixes found during validation (not Unity-specific)

Each is scoped so it cannot trade a false positive for the much costlier false negative:

  • account-no-delete: register.*userregister.*user\b, so UIKit's registerDefaults:@{@"UserAgent"...} no longer reads as signup (registerUser( still matches)
  • uiwebview-removed: new deadCodeGuards suppress matches inside a deployment-target branch (__IPHONE_OS_VERSION_MIN_REQUIRED < __IPHONE_9_0) that no shipping build compiles — e.g. unity-webview's compatibility shim. Suppression is line-scoped through real #if/#else/#endif tracking (internal/codescan/preproc.go): usage after the #endif, in the #else branch, before the guard, or anywhere in a file with unbalanced directives is still reported. Every ambiguity resolves toward "live", so a hard-rejection rule is never silently disabled
  • social-login-no-apple: auth SDKs abstract SIWA behind provider constants (APPLE_SIGNIN_RESULT_*) without ever naming ASAuthorization*; apple.*sign.*in now counts as the anti-pattern, mirroring the existing google.*sign.*in trigger. Anti-pattern matching now ignores comments project-wide, so // TODO: Sign in with Apple not supported yet can no longer satisfy the rule it contradicts — string literals still count, since SDK-driven implementations name their providers in strings

Tests

go test ./... passes. New coverage in internal/codescan/csharp_unity_test.go and internal/privacy/csharp_unity_test.go for each behavior above, including the adversarial cases: live UIWebView after #endif / in #else / with unbalanced directives, nested dead branches, a TODO comment failing to satisfy the SIWA anti-pattern, // inside a URL string literal, the non-Unity Library/ non-skip, and the PROVIDER_APPLE = "apple" non-match guard.

🤖 Generated with Claude Code


Note

Medium Risk
Changes affect App Review–critical heuristics (UIWebView, SIWA, IAP restore, account deletion) and broaden what code is scanned; behavior is heavily tested but regex/preprocessor logic could still miss edge cases.

Overview
Adds C# / Unity as a first-class scan target: .cs files are walked (with root-level Library/, Temp/, etc. skipped only when ProjectSettings/ProjectSettings.asset marks a Unity project), flow rules and DetectClaims learn Unity IAP / restore / SIWA patterns, and the privacy scanner treats PlayerPrefs, .NET file timestamps, and Unity ATT/post-build plist injection like their native equivalents.

Rule accuracy changes (scoped to avoid trading false positives for silent misses on critical rules): account-no-delete tightens signup detection; uiwebview-removed gains line-scoped #if dead-branch suppression via preproc.go; global anti-pattern checks strip comments (including multiline blocks) while keeping string-literal SDK evidence; SIWA / restore / platform-reference patterns are tuned for Unity and auth SDK constants.

Reviewed by Cursor Bugbot for commit c753348. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread internal/codescan/rules.go Outdated
Comment thread internal/codescan/rules.go
Unity is the dominant engine for mobile games, but greenlight's code scan
covered only Swift/ObjC/JS/TS — on a Unity project the flow rules judged the
app by its native plugin shims alone, producing both false positives and
blind spots.

- detectLanguage: recognize .cs as csharp (codescan + privacy scanner)
- rules: extend the generic rules to csharp; teach iap-no-restore Unity IAP
  (UnityEngine.Purchasing, IStoreListener, IAppleExtensions.RestoreTransactions)
- claims: same patterns, so the runtime tier sees Unity flows too
- privacy: PlayerPrefs is NSUserDefaults-backed on Apple platforms — count it
  as the User Defaults required-reason API; recognize Unity's ATT binding and
  post-build NSUserTrackingUsageDescription injection
- skip Unity-generated dirs (Library/Temp/Logs/obj/UserSettings) — but only
  when ProjectSettings/ProjectSettings.asset proves the root is a Unity project

Three false-positive fixes found while validating, each scoped so it cannot
trade a false positive for the much costlier false negative:

- account-no-delete: require a trailing word boundary so UIKit's
  registerDefaults:@{@"UserAgent"...} no longer reads as account signup
  (registerUser( still matches)
- uiwebview-removed: new deadCodeGuards suppress matches inside a
  deployment-target branch (__IPHONE_OS_VERSION_MIN_REQUIRED < __IPHONE_9_0)
  that no shipping build compiles. Suppression is line-scoped via real
  #if/#else/#endif tracking — usage after the #endif, in the #else branch, or
  in a file with unbalanced directives is still reported
- social-login-no-apple: recognize SIWA via provider constants
  (APPLE_SIGNIN_RESULT_*), mirroring the google.*sign.*in trigger. Anti-pattern
  matching now ignores comments project-wide, so `// TODO: Sign in with Apple
  not supported` can no longer satisfy the rule it contradicts; string literals
  still count, since SDK-driven implementations name providers in strings

Validated against a production Unity mobile game (~13k C# files): three false
positives eliminated, and the C# pass surfaced a real missing-privacy-manifest
issue (PlayerPrefs without NSPrivacyAccessedAPICategoryUserDefaults) that the
Swift-only scan could not see.
@treenod-ollie
treenod-ollie force-pushed the feature/csharp-unity-support branch from 67a2fb6 to 8cc74d7 Compare August 7, 2026 02:06

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.

There are 4 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8cc74d7. Configure here.

Comment thread internal/codescan/rules.go Outdated
Comment thread internal/codescan/rules.go
Comment thread internal/codescan/scanner.go Outdated
Four correctness issues from Cursor Bugbot on RevylAI#29. Each was a false negative —
a rule silently disabling itself — which is the costlier direction for a
scanner whose job is to catch rejection reasons.

- iap-no-restore: drop bare IAppleExtensions from the restore anti-pattern.
  That interface also carries deferred purchases, receipts and promo helpers,
  so reading a receipt looked like implementing restore. Only the actual
  RestoreTransactions call counts now
- social-login-no-apple: 'apple.*sign.*in' also matched Apple *code signing*
  ('appleSigning', 'signing with apple'), which appears in essentially every
  iOS build script and would disable the 4.8 rule project-wide. Tightened to
  apple[_\s-]*sign[_\s-]?in([^gG]|$) so sign-IN matches and sign-ING does not
- AntiPatternMatched: comment stripping was line-local, so continuation lines
  of a /* … */ block stayed exposed and a multi-line TODO could still satisfy
  an anti-pattern. Stripping now carries block state across the file
- Unity generated dirs: matched by basename at every depth, so real game
  source under Assets/Scripts/Logs or a plugin's own Library/ was skipped.
  Now matched by full path, so only the editor's root-level output is skipped

Tests cover each: receipt-only IAppleExtensions, code-signing text vs real
SIWA provider constants, a two-line block comment, and a .cs file under
Assets/Scripts/Logs surviving collection while root Library/ is skipped.
@treenod-ollie

Copy link
Copy Markdown
Author

Thanks Bugbot — all four were real, and all four were false negatives (a rule quietly disabling itself), which is the costlier direction here. Fixed in c753348.

Finding Fix
Bare IAppleExtensions counts as restore Dropped from the anti-pattern. That interface also carries deferred purchases, receipts and promo helpers, so reading a receipt read as implementing restore. Only RestoreTransactions counts now
SIWA pattern matches code signing apple.*sign.*in also matched appleSigning / "signing with apple", which appears in essentially every iOS build script and would disable 4.8 project-wide. Tightened to `apple[\s-]*sign[\s-]?in([^gG]
Block comments defeat anti-pattern strip Comment stripping was line-local, leaving /* … */ continuation lines exposed. It now carries block state across the file
Unity skips nested common folders Was basename matching at every depth, so Assets/Scripts/Logs or a plugin's own Library/ got skipped. Now matched by full path — only the editor's root-level output is skipped

The "guard text suppresses whole file" finding was already addressed before this review landed: fileSuppressPatterns is gone, replaced by real #if/#else/#endif tracking in internal/codescan/preproc.go that suppresses only the guarded branch. Usage after the #endif, in the #else branch, or in a file with unbalanced directives is still reported, and every ambiguity resolves toward "live".

Each fix has a regression test. Re-validated against the same production Unity project: findings unchanged, no false positive resurrected.

@ethanzhoucool

Copy link
Copy Markdown
Contributor

Ran this branch against a binary built from main to diff actual scan output. The C# work itself holds up: PlayerPrefs maps to NSPrivacyAccessedAPICategoryUserDefaults, Unity IAP without restore gets caught, and the Library/ skip is gated correctly (a hardcoded secret under Library/ is skipped in a Unity project and still flagged CRITICAL in the identical tree with ProjectSettings/ removed). Nice work on the gating, that was the part most likely to open a blind spot and it doesn't.

The rule changes are where I'd hold off. Four of them turn a finding main catches into silence. Each below is a reproduction, PR binary vs main, not a code-reading guess.

1. Dead-branch suppression inverts on compound conditions

preproc.go matches the guard as a substring of the #if line, so negation and || flip the meaning:

#if !(__IPHONE_OS_VERSION_MIN_REQUIRED < __IPHONE_9_0)
    UIWebView *live = [[UIWebView alloc] initWithFrame:CGRectZero];
#endif

main: CRITICAL 2.5.1. This branch: no issues. That code compiles on every modern deployment target. Same result for #if defined(LEGACY_SHIM) || __IPHONE_OS_VERSION_MIN_REQUIRED < __IPHONE_9_0, which ships whenever the flag is set.

The doc comment says every ambiguity resolves toward live. An unevaluatable condition that merely contains the guard resolves toward dead, which switches off a hard rejection rule. Fix is to anchor the guard to the entire condition and treat anything with !, ||, or && as live.

2. NSUserTrackingUsageDescription is treated as proof ATT is implemented

// src/analytics.js
import * as Analytics from 'firebase-analytics';
Analytics.init({ adId: true });
// app.config.js
ios: { infoPlist: { NSUserTrackingUsageDescription: 'We use your data to personalize ads' } }

main: HIGH 5.1.2. This branch: no issues. The key proves a purpose string exists, not that requestTrackingAuthorization is ever called. Declaring it without prompting is the most common ATT rejection there is, so this is the one I'd fix first. ATTrackingStatusBinding is a fair addition, the plist key is not. Note TestUnityATTRecognized currently locks this in as intended behavior.

3. register.*user\b drops camelCase signup identifiers

api.registerUserWithEmail(e, p)   // main: HIGH 5.1.1   PR: nothing
api.registerUser(e, p)            // both: HIGH 5.1.1

\b fails whenever user is followed by a word character, which covers most real signup identifiers including the RegisterUserAsync shape common in the C# code this PR adds support for. DetectClaims loses AccountCreation too, so the runtime tier stops exercising the flow. register[A-Za-z_]*user still fixes the registerDefaults:@{@"UserAgent"...} false positive you were chasing and keeps all of the above.

4. apple.*sign.*in matches text that says SIWA is absent

export const banner = "Sign in with Apple coming soon!";
export function googleSignIn() { return auth.signInWithGoogle(); }

main: HIGH 4.8. This branch: warn only. .* spans the whole line, so a docs URL or an appleSignIn: false flag satisfies the rule as well. Keeping string literals in scope makes sense for APPLE_SIGNIN_RESULT_*, the alternation just needs to be narrow enough that prose and disabled flags don't count.

5. Comment stripping leaks on escaped quotes

void G() { GoogleSignin.SignIn(); }
string s = "quote: \" end";  // TODO: implement Sign in with Apple

4.8 is suppressed project wide. The \" leaves stripCommentsMultiline in a bogus in-string state, so the trailing comment survives and satisfies the rule it contradicts. char q = '\''; does the same. Needs \ escape handling, and C# verbatim strings while you're in there.

One more I did not reproduce, flagging it as a lead rather than a finding: findPrivacyManifest in privacy/scanner.go doesn't apply the UnityGeneratedDirs skip that the required-reason walk does. If that's right, a PrivacyInfo.xcprivacy shipped inside Library/PackageCache/ would be read as the app's manifest, which would hide both privacy CRITICALs in exactly the Unity projects this PR targets. Can you check?

go build, go vet, go test ./... and -race all pass on the branch. The suite stays green through all five of the above, so each fix needs a test for the specific input.

The direction here is right and the false positives you found are real. The thing I care about is that a false negative costs more than a false positive for this tool: a spurious finding wastes someone ten minutes, a missing one loses them a review cycle. Every fix above keeps your false positive fix intact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants