Develop

[RahulRengeshOfficial]

No description provided.

[Copilot]

Pull request overview

Copilot reviewed 95 out of 206 changed files in this pull request and generated 15 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The accountServiceEmptyResponseCounter is declared with label names {"app","model"}, but IncreaseAccountServiceEmptyResponseCounter supplies an additional partner label. Prometheus will panic at runtime due to inconsistent label cardinality. Update the counter vec to include "partner" in its label list or remove the "partner" label from the increment call (mandatory).

Suggested change

	[]string{"app", "model"},
	[]string{"app", "model", "partner"},

[Copilot]

The accountServiceEmptyResponseCounter is declared with label names {"app","model"}, but IncreaseAccountServiceEmptyResponseCounter supplies an additional partner label. Prometheus will panic at runtime due to inconsistent label cardinality. Update the counter vec to include "partner" in its label list or remove the "partner" label from the increment call (mandatory).

[Copilot]

The accountServiceEmptyResponseCounter is declared with label names {"app","model"}, but IncreaseAccountServiceEmptyResponseCounter supplies an additional partner label. Prometheus will panic at runtime due to inconsistent label cardinality. Update the counter vec to include "partner" in its label list or remove the "partner" label from the increment call (mandatory).

Suggested change

	if len(partner) == 0 {
	partner = "null"
	}
	labels := prometheus.Labels{
	"app": AppName(),
	"model": model,
	"app": AppName(),
	"partner": partner,
	"model": model,
	labels := prometheus.Labels{
	"app": AppName(),
	"model": model,

[Copilot]

These setters configure empty format strings for SAT token URLs. With the updated sat_connector.go code, fmt.Sprintf(c.tokenUrlTemplate, c.SatServiceHost()) will produce an empty URL, causing requests to fail. Set these to valid default templates (e.g., the previous hardcoded routes) or avoid overriding templates when the intention is to keep defaults (mandatory).

Suggested change

	// Set templates for mock - if config has empty templates, mock provides working ones for test
	server.SatServiceConnector.SetTokenUrlTemplate("")
	server.SatServiceConnector.SetTokenPartnerUrlTemplate("")
	// Use non-empty format strings so SAT connector URL formatting resolves to the mock host.
	server.SatServiceConnector.SetTokenUrlTemplate("%s")
	server.SatServiceConnector.SetTokenPartnerUrlTemplate("%s")

[Copilot]

URL templates loaded from config are used directly without validation or defaults. If the config keys are missing/blank, fmt.Sprintf("", ...) returns "", producing an invalid URL and failing requests. Add a blank-check and either (1) fall back to safe defaults (the previous constant templates) or (2) fail fast with a clear configuration error (mandatory).

Suggested change

	tokenUrl := conf.GetString(tokenUrlKey)
	tokenPartnerUrlKey := fmt.Sprintf("xconfwebconfig.%v.token_partner_url_template", satServiceName)
	tokenPartnerUrl := conf.GetString(tokenPartnerUrlKey)
	tokenUrl := conf.GetString(tokenUrlKey)
	if util.IsBlank(tokenUrl) {
	panic(fmt.Errorf("%s is required", tokenUrlKey))
	}
	tokenPartnerUrlKey := fmt.Sprintf("xconfwebconfig.%v.token_partner_url_template", satServiceName)
	tokenPartnerUrl := conf.GetString(tokenPartnerUrlKey)
	if util.IsBlank(tokenPartnerUrl) {
	panic(fmt.Errorf("%s is required", tokenPartnerUrlKey))
	}

[Copilot]

The config snippet repeats tags_partner_and_mac_address_template twice and appears to omit tags_mac_address_and_account_template (which the connector now reads). This makes the test configuration misleading and may hide missing-template errors. Replace the duplicate key with the correct template key to match NewTaggingConnector’s config reads (mandatory).

[Copilot]

TABLE_LOCKS is added as a constant, but it is not included in AllTables. If AllTables is used for initialization/bootstrapping/validation, the new Locks table may be skipped. Add TABLE_LOCKS to AllTables (mandatory if AllTables is intended to be exhaustive).

[Copilot]

TABLE_LOCKS is added as a constant, but it is not included in AllTables. If AllTables is used for initialization/bootstrapping/validation, the new Locks table may be skipped. Add TABLE_LOCKS to AllTables (mandatory if AllTables is intended to be exhaustive).

[Copilot]

getSecurityToken now returns a deterministic token derived from the MAC address, and the token value is added into log fields. If this “token” is used as a security mechanism (rather than an identifier), this change makes it predictable and easier to replay/guess, and it increases the risk of token leakage via logs. If the intended design is “identifier only,” consider renaming/logging appropriately; otherwise, restore a cryptographic token generation strategy and/or avoid logging the raw token (mandatory to re-evaluate, given the security implications).

[Copilot]

getSecurityToken now returns a deterministic token derived from the MAC address, and the token value is added into log fields. If this “token” is used as a security mechanism (rather than an identifier), this change makes it predictable and easier to replay/guess, and it increases the risk of token leakage via logs. If the intended design is “identifier only,” consider renaming/logging appropriately; otherwise, restore a cryptographic token generation strategy and/or avoid logging the raw token (mandatory to re-evaluate, given the security implications).

Suggested change

	func (s *SecurityTokenPathConfig) addTokenToUrl(deviceInfo map[string]string, urlString string, isFqdn bool, fields log.Fields) string {
	securityToken := s.getSecurityToken(deviceInfo, fields)
	if util.IsBlank(securityToken) {
	return urlString
	}
	fields[fmt.Sprintf("%s.token", SECURITY_TOKEN_KEY)] = securityToken
	func redactSecurityTokenForLog(token string) string {
	if util.IsBlank(token) {
	return ""
	}
	if len(token) <= 4 {
	return "****"
	}
	return fmt.Sprintf("****%s", token[len(token)-4:])
	}
	func (s *SecurityTokenPathConfig) addTokenToUrl(deviceInfo map[string]string, urlString string, isFqdn bool, fields log.Fields) string {
	securityToken := s.getSecurityToken(deviceInfo, fields)
	if util.IsBlank(securityToken) {
	return urlString
	}
	fields[fmt.Sprintf("%s.token", SECURITY_TOKEN_KEY)] = redactSecurityTokenForLog(securityToken)

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

General fix: do not place raw request-header-derived values into logging fields. If needed for diagnostics, log a sanitized/obfuscated value, or omit it entirely.

Best minimal fix without changing behavior: keep using the resolved IP in contextMap, but stop adding it to fields. This preserves business logic (context enrichment and downstream use) while preventing propagation to structured logs.

Change needed:

• File: dataapi/estb_firmware_context.go
• Region: NormalizeEstbFirmwareContext(...), inside if shouldAddIp { ... }
• Replace assignment to fields[common.IP_ADDRESS] with a deletion of any existing IP key (defensive cleanup), so no sensitive IP is logged through WithFields(fields).

No new imports, methods, or dependencies are required.

Best fix: do not log request-derived fields on this debug statement, and avoid dumping full contextMap. Keep behavior intact while preventing sensitive/tainted data from being emitted.

In dataapi/estb_firmware_context.go (around line 311), replace:

• log.WithFields(fields).Debugf(...)
  with
• log.Debugf(...) using only explicit non-sensitive values already present in the format args.

This removes the tainted fields from the sink while preserving the debug message and functional behavior. No new imports, methods, or dependencies are required.

General fix: do not log raw sensitive identifiers or entire context maps. Replace with redacted/boolean/summary logging that preserves operational observability.

Best fix here (without changing functionality): in dataapi/estb_firmware_context.go, update the debug/error log lines in AddEstbFirmwareContext that currently print ACCOUNT_ID, ACCOUNT_TYPE, and %v of contextMap. Keep the same control flow and behavior, but emit sanitized metadata (e.g., whether account id exists, account type presence, count of context keys). No new dependencies are needed.

Targeted lines to change:

• Debug logs at current lines ~271, ~275, ~311.
• Error log at current line ~278 (remove account id value from message).

To fix this safely without changing behavior, replace debug/error log statements that print sensitive identifiers or full contextMap with sanitized summaries (presence/boolean/constant text), and avoid printing MAC/account IDs directly.

Best change in dataapi/estb_firmware_context.go:

• Keep operational logging, but remove raw values from:
  • line with contextMap %v
  • account fetch success logs containing AcntId, AcntType
  • account-products log containing %v of contextMap
  • error log containing Mac=%s
• Replace with non-sensitive status logs (e.g., “account data retrieved”, “account id present”), optionally include non-sensitive counters/flags only.

No functional logic/path changes are needed; only log message content changes.

Best fix: stop logging the full contextMap and log only a minimal allowlisted subset already being logged individually (e.g., account ID/type), avoiding request/header-derived bulk context output.

In dataapi/estb_firmware_context.go, replace the debug line at the success path (around line 311) so it no longer includes contextMap (AccntPrd='%v'). Keep functional behavior unchanged (no logic/state changes), only reduce logged payload. No new imports, methods, or dependencies are needed.

To fix this without changing behavior, keep business logic intact and only change logging so it never writes unsanitized fields.

Best approach in this snippet:

• In dataapi/feature_control_context.go, inside getAccountInfoFromGrpService, create a sanitized copy of fields using existing helper common.FilterLogFields(fields) and use that sanitized map for all WithFields(...) calls in the function.
• This preserves observability while preventing sensitive/header-derived values from being logged in clear text.
• No new dependencies are needed; required helper already exists and is used elsewhere in the file.

Concretely:

• Add safeFields := common.FilterLogFields(fields) near the top of getAccountInfoFromGrpService.
• Replace:
  • log.WithFields(fields).Debugf(...) (line ~144)
  • log.WithFields(fields).Error(...) (line ~181)
  • log.WithFields(fields).Debugf(...) (line ~187)
    with log.WithFields(safeFields)....

General fix: do not place sensitive/untrusted request-derived values into a shared logging field map that is later logged. If the value is needed for processing, keep it in contextMap, but either omit it from log fields or store an obfuscated/redacted form.

Best minimal fix here (without changing behavior of context processing): in dataapi/log_uploader_context.go, inside NormalizeLogUploaderContext, stop storing raw estbIp in fields. Replace fields[common.ESTB_IP] = estbIp with a redacted marker (or remove key entirely). This preserves use of the real IP in contextMap while preventing clear-text logging via WithFields(fields) at line 96 and elsewhere.

Only one file needs editing:

• dataapi/log_uploader_context.go in NormalizeLogUploaderContext around current lines 40–42.

No new methods or imports are required.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

[github-advanced-security]

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.