Skip to content

fix(plugin): prevent nil pointer dereference when validating manifest without auth#2717

Open
nankingjing wants to merge 1 commit into
coze-dev:mainfrom
nankingjing:fix-plugin-manifest-nil-auth
Open

fix(plugin): prevent nil pointer dereference when validating manifest without auth#2717
nankingjing wants to merge 1 commit into
coze-dev:mainfrom
nankingjing:fix-plugin-manifest-nil-auth

Conversation

@nankingjing

Copy link
Copy Markdown

Summary

PluginManifest.Validate dereferences mf.Auth.Type before any nil check on mf.Auth, so a manifest whose JSON omits the auth field (or sets it to null) causes a nil pointer dereference (panic) instead of a validation error.

Root cause

Auth is a nilable pointer field (Auth *AuthV2). Two other methods in the same file already guard it:

  • EncryptAuthPayload returns early on mf == nil || mf.Auth == nil.
  • validateAuthInfo returns an "auth is required" validation error when mf.Auth == nil.

However, Validate reaches if mf.Auth.Type == consts.AuthzTypeOfNone (the auth.type == none short-circuit) before it ever calls validateAuthInfo, so that existing nil guard is dead code for the "auth omitted" path.

Trigger

PluginApplicationService.RegisterPlugin unmarshals the caller-supplied ai_plugin JSON straight into the manifest and then calls CreateDraftPluginWithCode -> Manifest.Validate(false):

  • backend/application/plugin/registration.go: sonic.UnmarshalString(req.AiPlugin, &mf) (only LogoURL is set afterwards)
  • backend/domain/plugin/service/plugin_draft.go: req.Manifest.Validate(false)

A manifest that sets schema_version, the name/description fields and a valid api.type but has no auth key passes every check up to the mf.Auth.Type access and then panics. The same path is reachable via UpdateDraftPluginWithCode and the startup config loader (Validate(true)).

Fix

Add a nil check for mf.Auth before the auth.type == none short-circuit, returning the same "auth is required" error that validateAuthInfo already returns for a nil Auth. This turns the panic into the intended validation error and preserves existing behavior for every non-nil case.

Verification

Verified by reading and tracing the call path; not executed (no Go toolchain was available in my environment). The change is a small guard that reuses the errorx/errno symbols already imported and used in this file.

@nankingjing

Copy link
Copy Markdown
Author

Review: fix-plugin-manifest-nil-auth (#2717)

Verdict: LGTM

The fix adds a nil check for mf.Auth before mf.Auth.Type is accessed in Validate(). Without this, a manifest with no auth field would panic on mf.Auth.Type instead of returning a descriptive error.

Note that validateAuthInfo() already has an identical nil guard, but it is only reached AFTER the mf.Auth.Type check in Validate() -- so it never gets a chance to fire when mf.Auth is nil. The fix correctly places the guard before any Auth dereference.

Observation: The validation treats Auth as required for all manifests. If there is a legitimate use case for manifests without any auth configuration, that should be handled separately (e.g., treat nil as equivalent to AuthzTypeOfNone). But as a defensive nil-safety fix, this is the correct approach -- it replaces a panic with a clear error.

No issues found. Merge-ready.

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.

1 participant