Skip to content

enforce database timeouts#1781

Merged
AkramBitar merged 1 commit into
mainfrom
1633_enforce_timeout
Jul 10, 2026
Merged

enforce database timeouts#1781
AkramBitar merged 1 commit into
mainfrom
1633_enforce_timeout

Conversation

@HayimShaul

@HayimShaul HayimShaul commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Set a timeout to database queries to protect against database queries that are maliciously designed to take a long time.
Addresses issue #1633

@HayimShaul HayimShaul added this to the Q3/26 milestone Jun 8, 2026
@HayimShaul HayimShaul self-assigned this Jun 8, 2026
@HayimShaul HayimShaul linked an issue Jun 8, 2026 that may be closed by this pull request
@HayimShaul HayimShaul force-pushed the 1633_enforce_timeout branch 2 times, most recently from 64a1a10 to 5ba3782 Compare June 14, 2026 13:49
@HayimShaul HayimShaul marked this pull request as ready for review June 15, 2026 07:26
Comment thread docs/security/database_timeouts.md Outdated
Comment thread docs/security/database_timeouts.md Outdated
Comment thread docs/security/database_timeouts.md Outdated
Comment thread token/services/storage/db/sql/common/tokenlock.go Outdated
Comment thread token/services/storage/db/sql/common/timeout.go Outdated
Comment thread token/services/storage/db/sql/common/tokenlock.go
@AkramBitar

AkramBitar commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

The query timeout issue is tracked also under issue: #1740
This issue suggest to add the query timeout in the database level as a configuration parameter, while other solution can be in the application level like this PR suggests.

The timeout of the query can be defined for PostgreSQL in the database level but for SQLite we do not have this option.

Need to discuss with @adecaro and @mbrandenburger which approach is better.

PostgreSQL:

ALTER ROLE app_user
SET statement_timeout = '30s';

Go:

ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel() 

@AkramBitar

Copy link
Copy Markdown
Contributor

The query timeout issue is tracked also under issue: #1740 This issue suggest to add the query timeout in the database level as a configuration parameter, while other solution can be in the application level like this PR suggests.

The timeout of the query can be defined for PostgreSQL in the database level but for SQLite we do not have this option.

Need to discuss with @adecaro and @mbrandenburger which approach is better.

PostgreSQL:

ALTER ROLE app_user
SET statement_timeout = '30s';

Go:

ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel() 

After consulting with @adecaro,
We need to make sure that token sdk respects the context timeout.
In case we have a context timeout (or when the context canceled) then the query should be interrupted ASAP.

@HayimShaul HayimShaul force-pushed the 1633_enforce_timeout branch from cc3b877 to f3ee9fe Compare June 24, 2026 13:59
@AkramBitar

Copy link
Copy Markdown
Contributor

@HayimShaul
The context is indeed passed through to ExecContext(ctx, ...)/QueryContext(ctx, ...), so cancellation can propagate to the driver.

Should we add a test that runs a slow blocking query (e.g. pg_sleep/SLEEP or a fake blocking driver) and asserts that QueryContext/ExecContext is actually interrupted and returns context.Canceled or context.DeadlineExceeded instead of hanging?

@HayimShaul HayimShaul force-pushed the 1633_enforce_timeout branch 2 times, most recently from e941f43 to 225e82c Compare July 9, 2026 13:20

@AkramBitar AkramBitar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment thread token/services/storage/db/sql/common/tokens.go
@AkramBitar AkramBitar force-pushed the 1633_enforce_timeout branch from 225e82c to de61025 Compare July 10, 2026 12:05
Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com>
@AkramBitar AkramBitar force-pushed the 1633_enforce_timeout branch from de61025 to 3a0e497 Compare July 10, 2026 20:28
@AkramBitar AkramBitar merged commit 130b26f into main Jul 10, 2026
136 of 137 checks passed
@AkramBitar AkramBitar deleted the 1633_enforce_timeout branch July 10, 2026 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enforce Session and Operation Timeouts [HIGH]

3 participants