-
Notifications
You must be signed in to change notification settings - Fork 1
fix(adhoc-sweep-fixes): CU-86akbhhtv 66 review findings across 40 files #143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
32669f5
8910d32
829e162
43aa5af
c30afed
d1cb2e6
2729e85
5ff18cd
520d5f4
b23ceca
2881ec6
dda5091
d5b6657
05ac61f
aa72d31
65c8ab9
a75eb13
4eeecd8
7666425
fd1fc7a
131de62
9834aec
61af159
4a2ef5a
814f258
e79f03e
4f944fc
9cde661
3e4074c
a18d5f3
9005494
5cea3a2
c411d40
469daeb
e659e5f
8807c9a
c777b62
4d74b30
08d96b9
fa3ef2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -360,9 +360,11 @@ func TestHostVitalsLabelMembershipCronIDP(t *testing.T) { | |
| hosts := make([]*fleet.Host, 3) | ||
| teamIDs := []*uint{&team1.ID, &team2.ID, nil} | ||
| for i := range 3 { | ||
| osqueryHostID := fmt.Sprintf("idp-cron-%d", i) | ||
| nodeKey := fmt.Sprintf("idp-cron-%d", i) | ||
| h, err := ds.NewHost(ctx, &fleet.Host{ | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ cron_test.go calls new(...) as if it were a helper function for creating pointers, but this is not valid Go without a locally-defined helper In π€ Prompt for AI agentsfix confidence: π‘ 80 medium β react π/π to teach the reviewer |
||
| OsqueryHostID: new(fmt.Sprintf("idp-cron-%d", i)), | ||
| NodeKey: new(fmt.Sprintf("idp-cron-%d", i)), | ||
| OsqueryHostID: &osqueryHostID, | ||
| NodeKey: &nodeKey, | ||
| UUID: fmt.Sprintf("idp-cron-uuid%d", i), | ||
| Hostname: fmt.Sprintf("idp-cron-host%d.local", i), | ||
| HardwareSerial: fmt.Sprintf("idp-cron-hwd%d", i), | ||
|
|
@@ -376,9 +378,10 @@ func TestHostVitalsLabelMembershipCronIDP(t *testing.T) { | |
| // All three SCIM users are in the same "Engineering" IdP group. | ||
| scimUserIDs := make([]uint, 3) | ||
| for i := range 3 { | ||
| active := true | ||
| id, err := ds.CreateScimUser(ctx, &fleet.ScimUser{ | ||
| UserName: fmt.Sprintf("idp-cron-user%d", i), | ||
| Active: new(true), | ||
| Active: &active, | ||
| }) | ||
| require.NoError(t, err) | ||
| scimUserIDs[i] = id | ||
|
|
@@ -393,26 +396,29 @@ func TestHostVitalsLabelMembershipCronIDP(t *testing.T) { | |
| _, err = ds.CreateScimGroup(ctx, &fleet.ScimGroup{DisplayName: "Engineering", ScimUsers: scimUserIDs}) | ||
| require.NoError(t, err) | ||
|
|
||
| vital := "end_user_idp_group" | ||
| value := "Engineering" | ||
| criteria, err := json.Marshal(&fleet.HostVitalCriteria{ | ||
| Vital: new("end_user_idp_group"), | ||
| Value: new("Engineering"), | ||
| Vital: &vital, | ||
| Value: &value, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| // Create a global and a team1-scoped IdP host vitals label. | ||
| criteriaRawMessage := json.RawMessage(criteria) | ||
| globalLabel, err := ds.NewLabel(ctx, &fleet.Label{ | ||
| Name: "idp-cron-global", | ||
| LabelType: fleet.LabelTypeRegular, | ||
| LabelMembershipType: fleet.LabelMembershipTypeHostVitals, | ||
| HostVitalsCriteria: new(json.RawMessage(criteria)), | ||
| HostVitalsCriteria: &criteriaRawMessage, | ||
| }) | ||
| require.NoError(t, err) | ||
| team1Label, err := ds.NewLabel(ctx, &fleet.Label{ | ||
| Name: "idp-cron-team1", | ||
| TeamID: &team1.ID, | ||
| LabelType: fleet.LabelTypeRegular, | ||
| LabelMembershipType: fleet.LabelMembershipTypeHostVitals, | ||
| HostVitalsCriteria: new(json.RawMessage(criteria)), | ||
| HostVitalsCriteria: &criteriaRawMessage, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ unmount_network_filesystems() { | |
| mnt=$(printf '%b' "$mnt_esc") | ||
| # Never unmount critical mountpoints that may contain required userland. | ||
| case "$mnt" in | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ linux_wipe.sh unmounts network filesystems with a blocklist that misses many other essential paths In π€ Prompt for AI agentsfix confidence: π‘ 80 medium β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ linux_wipe.sh unmounts network filesystems with a blocklist that misses many other essential paths In π€ Prompt for AI agentsfix confidence: π‘ 80 medium β react π/π to teach the reviewer |
||
| /|/usr|/bin|/sbin|/lib|/lib64|/usr/bin|/usr/sbin|/usr/lib|/usr/lib64) | ||
| /|/usr|/bin|/sbin|/lib|/lib64|/usr/bin|/usr/sbin|/usr/lib|/usr/lib64|/etc|/var|/opt|/srv) | ||
| echo "Skipping critical network-mounted filesystem: $mnt" | ||
| continue | ||
| ;; | ||
|
|
@@ -230,3 +230,4 @@ else | |
| echo "Wiping, system will be unreachable" | ||
| (/usr/bin/nohup sh $0 wipe >/dev/null 2>/dev/null </dev/null) & | ||
| fi | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ const ApiOnlyUser = ({ router }: IApiOnlyUserProps): JSX.Element => { | |
| } | ||
| } catch (response) { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ console.error used to swallow fetch-current-user failure instead of surfacing to the user In the π€ Prompt for AI agentsfix confidence: π‘ 80 medium β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ console.error used to swallow fetch-current-user failure instead of surfacing to the user In the π€ Prompt for AI agentsfix confidence: π‘ 80 medium β react π/π to teach the reviewer |
||
| console.error(response); | ||
| router.push(LOGIN); | ||
| } | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,16 +99,20 @@ const UsersForm = ({ | |
| e.preventDefault(); | ||
|
|
||
| setIsUpdating(true); | ||
| const canLockEndUserInfo = | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ UsersForm resets lockEndUserInfo to computed canLockEndUserInfo after save even when Apple MDM is not configured In π€ Prompt for AI agentsfix confidence: π‘ 85 medium β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ UsersForm resets lockEndUserInfo to computed canLockEndUserInfo after save even when Apple MDM is not configured In π€ Prompt for AI agentsfix confidence: π‘ 85 medium β react π/π to teach the reviewer |
||
| formData.endUserAuthEnabled && formData.lockEndUserInfo; | ||
| // Only collapse lockEndUserInfo based on endUserAuthEnabled when Apple | ||
| // MDM is configured. Otherwise the checkbox is read-only and reflects a | ||
| // value preserved from the backend, so it should be sent as-is. | ||
| const lockEndUserInfoToSend = isMacMdmEnabledAndConfigured | ||
| ? formData.endUserAuthEnabled && formData.lockEndUserInfo | ||
| : formData.lockEndUserInfo; | ||
|
|
||
| try { | ||
| await mdmAPI.updateSetupExperienceSettings({ | ||
| fleet_id: currentTeamId, | ||
| enable_end_user_authentication: formData.endUserAuthEnabled, | ||
| // Apple-only fields are omitted when Apple MDM isn't configured. | ||
| ...(isMacMdmEnabledAndConfigured && { | ||
| lock_end_user_info: canLockEndUserInfo, | ||
| lock_end_user_info: lockEndUserInfoToSend, | ||
| enable_managed_local_account: effectiveEnableManagedLocalAccount( | ||
| formData | ||
| ), | ||
|
|
@@ -122,7 +126,10 @@ const UsersForm = ({ | |
|
|
||
| setIsUpdating(false); | ||
| if (isMacMdmEnabledAndConfigured) { | ||
| setFormData((prev) => ({ ...prev, lockEndUserInfo: canLockEndUserInfo })); | ||
| setFormData((prev) => ({ | ||
| ...prev, | ||
| lockEndUserInfo: lockEndUserInfoToSend, | ||
| })); | ||
| } | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ const MfaPage = ({ router, params }: IMfaPage) => { | |
| } = useContext(AppContext); | ||
| const { redirectLocation } = useContext(RoutingContext); | ||
| const [isExpired, setIsExpired] = useState(false); | ||
| const [hasError, setHasError] = useState(false); | ||
| const [shouldFinishMFA, setShouldFinishMFA] = useState( | ||
| !!local.getItem("auth_pending_mfa") | ||
| ); | ||
|
|
@@ -73,7 +74,12 @@ const MfaPage = ({ router, params }: IMfaPage) => { | |
| router.push(redirectLocation || DASHBOARD); | ||
| }); | ||
| } catch (response) { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ finishMFA swallows all errors from the MFA completion API into a single 'expired' state, masking other failure causes In π€ Prompt for AI agentsfix confidence: π΄ 55 low β review closely β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ finishMFA swallows all errors from the MFA completion API into a single 'expired' state, masking other failure causes In π€ Prompt for AI agentsfix confidence: π΄ 55 low β review closely β react π/π to teach the reviewer |
||
| setIsExpired(true); | ||
| const status = (response as { status?: number })?.status; | ||
| if (status === 401 || status === 410) { | ||
| setIsExpired(true); | ||
| } else { | ||
| setHasError(true); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
|
|
@@ -118,6 +124,19 @@ const MfaPage = ({ router, params }: IMfaPage) => { | |
| ); | ||
| } | ||
|
|
||
| if (hasError) { | ||
| return ( | ||
| <AuthenticationFormWrapper className={baseClass} header="Something went wrong"> | ||
| <> | ||
| <div className={`${baseClass}__description`}> | ||
| <p>An error occurred. Please try again.</p> | ||
| </div> | ||
| <Button onClick={onClickLoginButton}>Back to login</Button> | ||
| </> | ||
| </AuthenticationFormWrapper> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <AuthenticationFormWrapper className={baseClass}> | ||
| <Spinner /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,7 +51,7 @@ const AddAbmModal = ({ onCancel, onAdded }: IAddAbmModalProps) => { | |
| }, [tokenFile, renderFlash, onAdded, onCancel]); | ||
|
|
||
| return ( | ||
| <Modal className={baseClass} title="Add AB" onExit={onCancel} width="large"> | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ AddAbmModal title truncated to 'Add AB' and button text 'Add AB' β appears to be a search/replace error dropping 'M' from 'ABM' Changed π€ Prompt for AI agentsfix confidence: π‘ 65 medium β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ AddAbmModal title truncated to 'Add AB' and button text 'Add AB' β appears to be a search/replace error dropping 'M' from 'ABM' Changed π€ Prompt for AI agentsfix confidence: π‘ 65 medium β react π/π to teach the reviewer |
||
| <Modal className={baseClass} title="Add ABM" onExit={onCancel} width="large"> | ||
| <p> | ||
| Follow the step-by-step guide to connect Fleet to Apple Business.{" "} | ||
| <CustomLink | ||
|
|
@@ -78,7 +78,7 @@ const AddAbmModal = ({ onCancel, onAdded }: IAddAbmModalProps) => { | |
| isLoading={isUploading} | ||
| disabled={!tokenFile || isUploading} | ||
| > | ||
| Add AB | ||
| Add ABM | ||
| </Button> | ||
| <DownloadABMKey baseClass={baseClass} /> | ||
| </div> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,9 @@ const DeleteEntraClientIdModal = ({ | |
|
|
||
| try { | ||
| const currentClientIds = config?.mdm.windows_entra_client_ids ?? []; | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Entra client ID de-duplication normalizes input to lowercase but does not validate case-insensitive duplicates against the raw stored value consistently on delete Changed the filter predicate in π€ Prompt for AI agentsfix confidence: π‘ 85 medium β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ Entra client ID de-duplication normalizes input to lowercase but does not validate case-insensitive duplicates against the raw stored value consistently on delete Changed the filter predicate in π€ Prompt for AI agentsfix confidence: π‘ 85 medium β react π/π to teach the reviewer |
||
| const updatedClientIds = currentClientIds.filter((id) => id !== clientId); | ||
| const updatedClientIds = currentClientIds.filter( | ||
| (id) => id.toLowerCase() !== clientId.toLowerCase() | ||
| ); | ||
| const updateData = await configAPI.update({ | ||
| mdm: { | ||
| windows_entra_client_ids: updatedClientIds, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,10 @@ import { ShowActivityDetailsHandler } from "components/ActivityItem/ActivityItem | |
|
|
||
| import EmptyFeed from "../EmptyFeed/EmptyFeed"; | ||
|
|
||
| import { pastActivityComponentMap } from "../ActivityConfig"; | ||
| import { | ||
| pastActivityComponentMap, | ||
| IHostActivityItemComponentPropsWithShowDetails, | ||
| } from "../ActivityConfig"; | ||
|
|
||
| const baseClass = "past-activity-feed"; | ||
|
|
||
|
|
@@ -22,6 +25,12 @@ interface IPastActivityFeedProps { | |
| onPreviousPage: () => void; | ||
| } | ||
|
|
||
| const usesShowDetails = ( | ||
| activityType: keyof typeof pastActivityComponentMap | ||
| ): boolean => { | ||
| return Boolean(activityType); | ||
| }; | ||
|
|
||
| const PastActivityFeed = ({ | ||
| activities, | ||
| isError = false, | ||
|
|
@@ -68,13 +77,30 @@ const PastActivityFeed = ({ | |
| ); | ||
| return null; | ||
| } | ||
| if ( | ||
| "onShowDetails" in ActivityItemComponent.propTypes || | ||
| usesShowDetails(activity.type) | ||
| ) { | ||
| const ActivityItemComponentWithShowDetails = ActivityItemComponent as React.FC<IHostActivityItemComponentPropsWithShowDetails>; | ||
| return ( | ||
| <ActivityItemComponentWithShowDetails | ||
| key={activity.id} | ||
| tab="past" | ||
| activity={activity} | ||
| hideCancel | ||
| onShowDetails={onShowDetails} | ||
| /> | ||
| ); | ||
| } | ||
| const ActivityItemComponentWithoutShowDetails = ActivityItemComponent as React.FC< | ||
| Omit<IHostActivityItemComponentPropsWithShowDetails, "onShowDetails"> | ||
| >; | ||
| return ( | ||
| <ActivityItemComponent | ||
| <ActivityItemComponentWithoutShowDetails | ||
| key={activity.id} | ||
| tab="past" | ||
| activity={activity} | ||
| hideCancel | ||
| onShowDetails={onShowDetails} | ||
| /> | ||
| ); | ||
| })} | ||
|
Comment on lines
77
to
106
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ PastActivityFeed casts pastActivityComponentMap lookup to a component accepting show-details props without a type guard, but not all entries use IHostActivityItemComponentPropsWithShowDetails Attempted to address the type-safety gap in the π€ Prompt for AI agentsfix confidence: π΄ 25 low β review closely β react π/π to teach the reviewer
Comment on lines
77
to
106
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ PastActivityFeed casts pastActivityComponentMap lookup to a component accepting show-details props without a type guard, but not all entries use IHostActivityItemComponentPropsWithShowDetails Attempted to address the type-safety gap in the π€ Prompt for AI agentsfix confidence: π΄ 25 low β review closely β react π/π to teach the reviewer |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -177,9 +177,15 @@ const AutomationsModal = ({ | |
| await Promise.all(promises); | ||
| } else if (teamIdForApi !== undefined) { | ||
| // A real team: everything goes to teams.update in a single payload. | ||
| // Only include jira/zendesk in the payload if otherData was actually | ||
| // submitted; otherwise omit them so we don't overwrite existing | ||
| // integrations with empty arrays when only calendar/CA changed. | ||
| const integrations: ITeamIntegrations = { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ AutomationsModal team-update payload silently discards calendar/CA data when otherData is falsy In π€ Prompt for AI agentsfix confidence: π‘ 75 medium β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π΄ AutomationsModal team-update payload silently discards calendar/CA data when otherData is falsy In π€ Prompt for AI agentsfix confidence: π‘ 75 medium β react π/π to teach the reviewer |
||
| jira: otherData?.integrations.jira ?? [], | ||
| zendesk: otherData?.integrations.zendesk ?? [], | ||
| jira: otherData?.integrations.jira ?? teamConfig?.integrations.jira ?? [], | ||
| zendesk: | ||
| otherData?.integrations.zendesk ?? | ||
| teamConfig?.integrations.zendesk ?? | ||
| [], | ||
| }; | ||
| if (calendarData) { | ||
| integrations.google_calendar = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,7 @@ func rSign(pkgPath, cert string) error { | |
| defer os.Remove(pemPath) | ||
| err := os.WriteFile(pemPath, []byte(cert), 0o600) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π rSign uses %s instead of %w when wrapping the cert-write error In rSign, changed π€ Prompt for AI agentsfix confidence: π’ 90 high β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π rSign uses %s instead of %w when wrapping the cert-write error In rSign, changed π€ Prompt for AI agentsfix confidence: π’ 90 high β react π/π to teach the reviewer |
||
| if err != nil { | ||
| return fmt.Errorf("writing cert data: %s", err) | ||
| return fmt.Errorf("writing cert data: %w", err) | ||
| } | ||
|
|
||
| return retry.Do(func() error { | ||
|
|
@@ -66,19 +66,19 @@ func rNotarizeStaple(pkg, apiKeyID, apiKeyIssuer, apiKeyContent string) error { | |
| func writeAPIKeys(issuer, id, content string) (string, error) { | ||
| homedir, err := os.UserHomeDir() | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π writeAPIKeys wraps errors with %s instead of %w in three places In writeAPIKeys, changed all three fmt.Errorf calls (os.UserHomeDir, secure.MkdirAll, os.WriteFile) from π€ Prompt for AI agentsfix confidence: π‘ 85 medium β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π writeAPIKeys wraps errors with %s instead of %w in three places In writeAPIKeys, changed all three fmt.Errorf calls (os.UserHomeDir, secure.MkdirAll, os.WriteFile) from π€ Prompt for AI agentsfix confidence: π‘ 85 medium β react π/π to teach the reviewer |
||
| if err != nil { | ||
| return "", fmt.Errorf("finding home dir: %s", err) | ||
| return "", fmt.Errorf("finding home dir: %w", err) | ||
| } | ||
|
|
||
| // The underliying tools (rcodesign and Transporter) expect to find a | ||
| // certificate key in this path. | ||
| path := filepath.Join(homedir, ".appstoreconnect", "private_keys") | ||
| if err = secure.MkdirAll(path, 0o600); err != nil { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π writeAPIKeys returns wrong wrapped error message on MkdirAll failure In writeAPIKeys, changed the MkdirAll error wrap message from "finding home dir: %s" to "creating private keys dir: %w", fixing the copy-paste drift so the error message describes the actual failing operation. π€ Prompt for AI agentsfix confidence: π’ 90 high β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π secure.MkdirAll called with file-permission bits (0o600) instead of directory permission bits In writeAPIKeys, changed secure.MkdirAll permission bits from 0o600 to 0o700 to include the execute bit required for directory traversal. Risk: this changes on-disk permissions behavior for an existing directory path; if any deployed environment relies on the previous (arguably broken) permission value, this could theoretically alter observed permissions, though the fix aligns with correct POSIX directory semantics. π€ Prompt for AI agentsfix confidence: π‘ 75 medium β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π writeAPIKeys returns wrong wrapped error message on MkdirAll failure In writeAPIKeys, changed the MkdirAll error wrap message from "finding home dir: %s" to "creating private keys dir: %w", fixing the copy-paste drift so the error message describes the actual failing operation. π€ Prompt for AI agentsfix confidence: π’ 90 high β react π/π to teach the reviewer
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 𦩠π secure.MkdirAll called with file-permission bits (0o600) instead of directory permission bits In writeAPIKeys, changed secure.MkdirAll permission bits from 0o600 to 0o700 to include the execute bit required for directory traversal. Risk: this changes on-disk permissions behavior for an existing directory path; if any deployed environment relies on the previous (arguably broken) permission value, this could theoretically alter observed permissions, though the fix aligns with correct POSIX directory semantics. π€ Prompt for AI agentsfix confidence: π‘ 75 medium β react π/π to teach the reviewer |
||
| return "", fmt.Errorf("finding home dir: %s", err) | ||
| if err = secure.MkdirAll(path, 0o700); err != nil { | ||
| return "", fmt.Errorf("creating private keys dir: %w", err) | ||
| } | ||
|
|
||
| keyPath := filepath.Join(path, fmt.Sprintf("AuthKey_%s.p8", id)) | ||
| if err = os.WriteFile(keyPath, []byte(content), 0o600); err != nil { | ||
| return "", fmt.Errorf("writing api key contents: %s", err) | ||
| return "", fmt.Errorf("writing api key contents: %w", err) | ||
| } | ||
|
|
||
| return keyPath, nil | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
𦩠π΄ cron_test.go calls new(...) as if it were a helper function for creating pointers, but this is not valid Go without a locally-defined helper
In
TestHostVitalsLabelMembershipCronIDP, replaced all invalidnew(value)calls with standard Go pointer idioms: introduced local variables (osqueryHostID,nodeKey,active,vital,value,criteriaRawMessage) and took their addresses (&osqueryHostID, etc.) for theOsqueryHostID,NodeKey,Active,Vital,Value, andHostVitalsCriteriastruct fields, so the file now compiles without relying on any nonexistent genericnew[T any](v T) *Thelper. Risk: since no such helper is defined anywhere in the visible file, this assumes the original intent was simple pointer-to-value construction identical to what aptr.String/ptr.Bool-style helper would produce; the reviewer should confirm the field types (*string,*bool,*json.RawMessage) match, and check whether the project has a conventionalptrpackage that should be used instead for consistency with the rest of the codebase.π€ Prompt for AI agents
fix confidence: π‘ 80 medium β react π/π to teach the reviewer