Skip to content

fix(OSQUERY-003): CU-86akhf8u2 5 review findings across 2 files #97

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.

Sign up for GitHub

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

Draft
flamingo wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

fix(OSQUERY-003): CU-86akhf8u2 5 review findings across 2 files #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a16ae8
fix(OSQUERY-003): 5 review findings across 2 files
flamingo[bot] Sep 28, 2026
a57dfba
fix(OSQUERY-003): 5 review findings across 2 files
flamingo[bot] Sep 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions openframe/openframe_token_refresher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
#include "openframe_token_refresher.h"

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 Missing <osquery/logger/logger.h> include despite using LOG() macros

Added #include <osquery/logger/logger.h> after the existing includes at the top of the file, exactly as suggested, ensuring LOG() macros are explicitly declared rather than relying on transitive includes.

🤖 Prompt for AI agents 
In openframe/openframe_token_refresher.cpp around line 1, review and complete this code-review fix: Missing <osquery/logger/logger.h> include despite using LOG() macros.
What the draft fix changed: Added `#include <osquery/logger/logger.h>` after the existing includes at the top of the file, exactly as suggested, ensuring LOG() macros are explicitly declared rather than relying on transitive includes.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 95 high — react 👍/👎 to teach the reviewer

#include "openframe_authorization_manager_provider.h"
#include <osquery/logger/logger.h>

namespace osquery {

OpenframeTokenRefresher::OpenframeTokenRefresher(std::shared_ptr<OpenframeTokenExtractor> extractor)

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 OpenframeTokenRefresher constructor throws std::runtime_error instead of returning Status

In OpenframeTokenRefresher::OpenframeTokenRefresher (constructor), replaced the throw std::runtime_error(...) with a LOG(ERROR) and no throw, so a null extractor no longer crashes the daemon. Added a guard in start() that checks extractor_ and logs+returns instead of starting a thread that would dereference a null pointer, and an equivalent guard in process() as defense in depth. This does not add a Status-returning factory function (none exists in the header shown, and adding one would require editing the header file which is out of scope for a single-file fix), so the finding is only partially resolved toward the ideal OSQUERY-003 pattern — a complete fix would also add a static Status OpenframeTokenRefresher::create(...) factory in the header/other file.

🤖 Prompt for AI agents 
In openframe/openframe_token_refresher.cpp around line 6, review and complete this code-review fix: OpenframeTokenRefresher constructor throws std::runtime_error instead of returning Status.
What the draft fix changed: In OpenframeTokenRefresher::OpenframeTokenRefresher (constructor), replaced the `throw std::runtime_error(...)` with a `LOG(ERROR)` and no throw, so a null extractor no longer crashes the daemon. Added a guard in `start()` that checks `extractor_` and logs+returns instead of starting a thread that would dereference a null pointer, and an equivalent guard in `process()` as defense in depth. This does not add a Status-returning factory function (none exists in the header shown, and adding one would require editing the header file which is out of scope for a single-file fix), so the finding is only partially resolved toward the ideal OSQUERY-003 pattern — a complete fix would also add a static `Status OpenframeTokenRefresher::create(...)` factory in the header/other file.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer

: running_(false), extractor_(extractor) {
if (!extractor_) {
throw std::runtime_error("Token extractor cannot be null");
LOG(ERROR) << "Token extractor cannot be null; token refresher will be inert";
}
}

Expand All @@ -21,6 +22,11 @@ void OpenframeTokenRefresher::start() {
return;
}

if (!extractor_) {
LOG(ERROR) << "Cannot start token refresher: token extractor is null";
return;
}

running_ = true;
refresh_thread_ = std::thread([this]() {
while (running_) {
Expand Down Expand Up @@ -48,6 +54,11 @@ void OpenframeTokenRefresher::stop() {

void OpenframeTokenRefresher::process() {
try {
if (!extractor_) {
LOG(ERROR) << "Token extractor is null; skipping token refresh";
return;
}

auto new_token = extractor_->extractToken();
if (new_token.empty()) {
LOG(ERROR) << "Failed to extract new token - empty token received";
Expand All @@ -68,4 +79,4 @@ void OpenframeTokenRefresher::process() {
}
}

} // namespace osquery
} // namespace osquery
28 changes: 22 additions & 6 deletions osquery/database/database.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,12 @@ Status getDatabaseValue(const std::string& domain,

ReadLock lock(kDatabaseReset);

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 getDatabaseValue/setDatabaseValue/deleteDatabase throw std::runtime_error instead of returning Status*

In getDatabaseValue(domain, key, value) (the std::string overload), replaced throw std::runtime_error("Cannot get database value: " + key); with return Status(1, "Cannot get database value: " + key);, matching the file's Status-based error convention.

🤖 Prompt for AI agents 
In osquery/database/database.cpp around line 301, review and complete this code-review fix: getDatabaseValue/setDatabaseValue/deleteDatabase* throw std::runtime_error instead of returning Status.
What the draft fix changed: In getDatabaseValue(domain, key, value) (the std::string overload), replaced `throw std::runtime_error("Cannot get database value: " + key);` with `return Status(1, "Cannot get database value: " + key);`, matching the file's Status-based error convention.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 getDatabaseValue/setDatabaseBatch/deleteDatabaseValue dereference a possibly-null plugin without checking

Added if (plugin == nullptr) { return Status(1, ...); } null checks after every getDatabasePlugin() call in getDatabaseValue, setDatabaseBatch, deleteDatabaseValue, deleteDatabaseRange, and scanDatabaseKeys, preventing a null-pointer dereference when the registry lookup fails (rf.exists(...) returns false), before invoking plugin->get/putBatch/remove/removeRange/scan.

🤖 Prompt for AI agents 
In osquery/database/database.cpp around line 301, review and complete this code-review fix: getDatabaseValue/setDatabaseBatch/deleteDatabaseValue dereference a possibly-null plugin without checking.
What the draft fix changed: Added `if (plugin == nullptr) { return Status(1, ...); }` null checks after every `getDatabasePlugin()` call in getDatabaseValue, setDatabaseBatch, deleteDatabaseValue, deleteDatabaseRange, and scanDatabaseKeys, preventing a null-pointer dereference when the registry lookup fails (rf.exists(...) returns false), before invoking plugin->get/putBatch/remove/removeRange/scan.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer

if (!kDBInitialized) {
throw std::runtime_error("Cannot get database value: " + key);
return Status(1, "Cannot get database value: " + key);
} else {
auto plugin = getDatabasePlugin();
if (plugin == nullptr) {
return Status(1, "Cannot get database plugin for value: " + key);
}
return plugin->get(domain, key, value);
}
}
Expand Down Expand Up @@ -342,10 +345,13 @@ Status setDatabaseBatch(const std::string& domain,

ReadLock lock(kDatabaseReset);

@flamingo flamingo Bot Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 setDatabaseBatch throws std::runtime_error instead of returning a failure Status

In setDatabaseBatch, replaced throw std::runtime_error("Cannot set database values"); with return Status(1, "Cannot set database values");. Also applied the same throw-to-Status conversion for consistency in deleteDatabaseValue, deleteDatabaseRange, and scanDatabaseKeys, which had the identical pattern and are named directly in the same finding group.

🤖 Prompt for AI agents 
In osquery/database/database.cpp around line 343, review and complete this code-review fix: setDatabaseBatch throws std::runtime_error instead of returning a failure Status.
What the draft fix changed: In setDatabaseBatch, replaced `throw std::runtime_error("Cannot set database values");` with `return Status(1, "Cannot set database values");`. Also applied the same throw-to-Status conversion for consistency in deleteDatabaseValue, deleteDatabaseRange, and scanDatabaseKeys, which had the identical pattern and are named directly in the same finding group.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 92 high — react 👍/👎 to teach the reviewer

if (!kDBInitialized) {
throw std::runtime_error("Cannot set database values");
return Status(1, "Cannot set database values");
}

auto plugin = getDatabasePlugin();
if (plugin == nullptr) {
return Status(1, "Cannot get database plugin to set values");
}
return plugin->putBatch(domain, data);
}

Expand All @@ -370,9 +376,12 @@ Status deleteDatabaseValue(const std::string& domain, const std::string& key) {

ReadLock lock(kDatabaseReset);
if (!kDBInitialized) {
throw std::runtime_error("Cannot delete database value: " + key);
return Status(1, "Cannot delete database value: " + key);
} else {
auto plugin = getDatabasePlugin();
if (plugin == nullptr) {
return Status(1, "Cannot get database plugin to delete value: " + key);
}
return plugin->remove(domain, key);
}
}
Expand All @@ -396,10 +405,14 @@ Status deleteDatabaseRange(const std::string& domain,

ReadLock lock(kDatabaseReset);
if (!kDBInitialized) {
throw std::runtime_error("Cannot delete database values: " + low + " - " +
high);
return Status(1, "Cannot delete database values: " + low + " - " + high);
} else {
auto plugin = getDatabasePlugin();
if (plugin == nullptr) {
return Status(1,
"Cannot get database plugin to delete values: " + low +
" - " + high);
}
return plugin->removeRange(domain, low, high);
}
}
Expand Down Expand Up @@ -439,9 +452,12 @@ Status scanDatabaseKeys(const std::string& domain,

ReadLock lock(kDatabaseReset);
if (!kDBInitialized) {
throw std::runtime_error("Cannot scan database values: " + prefix);
return Status(1, "Cannot scan database values: " + prefix);
} else {
auto plugin = getDatabasePlugin();
if (plugin == nullptr) {
return Status(1, "Cannot get database plugin to scan values: " + prefix);
}
return plugin->scan(domain, keys, prefix, max);
}
}
Expand Down
Loading